Conversation
…ance check two-sided Widens the cost snapshot from 24 rows to 48, covering rga, lww_register and nested_map alongside the three collections #3660 landed with. Two findings came out of doing it: - ReplicatedGrowableArray::insert(pos, char) costs n + ~47 storage rows, so building a document one character at a time is quadratic. No existing CostShape could describe that honestly, so this adds QuadraticBuild with a ratchet that fails if the curve worsens and, with a different message, if it silently stops being quadratic. - reproducible.rs only ever failed a tolerance that was too tight, while three comments claimed it caught both directions. vector_get_nth therefore carried a 25% band against a measured spread of 10.5%, admitting a real 20% regression in Vector::get. The check is now two-sided and the band is 18%, proven by breaking a row and watching the gate catch what the old one passed. sorted_map is deliberately not registered: without RuntimeEnv::with_index wired, measuring it would attribute zero cost to all index maintenance while still printing plausible numbers. The reasoning is in the registry, not just in a review comment.
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
…tity id `row_counts_are_deterministic_across_calls` built its workload with `UnorderedMap::new()`, which mints a RANDOM entity id. A parent's children live in a hash trie (core#3633) whose descent depth follows the hash of that id, so two runs of an identical workload read a different number of trie nodes while writing exactly the same set. That is why only `rows_read` moved while `rows_written` sat at 313, and why it failed under `calimero-storage/testing` — which CI enables — roughly ALWAYS rather than occasionally: 0 pass / 12 fail locally before, 15/15 after. The sibling `byte_counts_are_not_reproducible` already records that ids are not deterministic; this test simply must not depend on them being so. `new_with_field_name` gives the collection a deterministic id, which is what the property under test actually needs. Measured: 15/15 under `--features calimero-storage/testing`, 10/10 under default features, and `./scripts/check-storage-cost.sh` still green on all 124 rows — the change touches only the test's own workload, never a measured row. This is the single failure shared by #3750, #3752 and #3754. It is not stale-base: the tool is byte-identical between `bench/05-gas-walls` and this branch, and it reproduces here. Claude-Session: https://claude.ai/code/session_014mxKpC4vXk9Bujuetq7KBk
…tity id `row_counts_are_deterministic_across_calls` built its workload with `UnorderedMap::new()`, which mints a RANDOM entity id. A parent's children live in a hash trie (core#3633) whose descent depth follows the hash of that id, so two runs of an identical workload read a different number of trie nodes while writing exactly the same set. That is why only `rows_read` moved while `rows_written` sat at 313, and why it failed under `calimero-storage/testing` — which CI enables — roughly ALWAYS rather than occasionally: 0 pass / 12 fail locally before, 15/15 after. The sibling `byte_counts_are_not_reproducible` already records that ids are not deterministic; this test simply must not depend on them being so. `new_with_field_name` gives the collection a deterministic id, which is what the property under test actually needs. Measured: 15/15 under `--features calimero-storage/testing`, 10/10 under default features, and `./scripts/check-storage-cost.sh` still green on all 124 rows — the change touches only the test's own workload, never a measured row. This is the single failure shared by #3750, #3752 and #3754. It is not stale-base: the tool is byte-identical between `bench/05-gas-walls` and this branch, and it reproduces here. Claude-Session: https://claude.ai/code/session_014mxKpC4vXk9Bujuetq7KBk
|
Pushed The bug. That accounts for both clues in the CI output: The fix is Measured on this branch, same command, with and without the change: plus 10/10 under default features and Two things I got wrong on the way, in case they are in anyone's notes: this is not stale-base (the tool is byte-identical between this branch and a master-merged one, and it reproduces on both), and it is not flaky — it fails essentially always under CI's feature set. |
…tity id `row_counts_are_deterministic_across_calls` built its workload with `UnorderedMap::new()`, which mints a RANDOM entity id. A parent's children live in a hash trie (core#3633) whose descent depth follows the hash of that id, so two runs of an identical workload read a different number of trie nodes while writing exactly the same set. That is why only `rows_read` moved while `rows_written` sat at 313, and why it failed under `calimero-storage/testing` — which CI enables — roughly ALWAYS rather than occasionally: 0 pass / 12 fail locally before, 15/15 after. The sibling `byte_counts_are_not_reproducible` already records that ids are not deterministic; this test simply must not depend on them being so. `new_with_field_name` gives the collection a deterministic id, which is what the property under test actually needs. Measured: 15/15 under `--features calimero-storage/testing`, 10/10 under default features, and `./scripts/check-storage-cost.sh` still green on all 124 rows — the change touches only the test's own workload, never a measured row. This is the single failure shared by #3750, #3752 and #3754. It is not stale-base: the tool is byte-identical between `bench/05-gas-walls` and this branch, and it reproduces here. Claude-Session: https://claude.ai/code/session_014mxKpC4vXk9Bujuetq7KBk
…tity id `row_counts_are_deterministic_across_calls` built its workload with `UnorderedMap::new()`, which mints a RANDOM entity id. A parent's children live in a hash trie (core#3633) whose descent depth follows the hash of that id, so two runs of an identical workload read a different number of trie nodes while writing exactly the same set. That is why only `rows_read` moved while `rows_written` sat at 313, and why it failed under `calimero-storage/testing` — which CI enables — roughly ALWAYS rather than occasionally: 0 pass / 12 fail locally before, 15/15 after. The sibling `byte_counts_are_not_reproducible` already records that ids are not deterministic; this test simply must not depend on them being so. `new_with_field_name` gives the collection a deterministic id, which is what the property under test actually needs. Measured: 15/15 under `--features calimero-storage/testing`, 10/10 under default features, and `./scripts/check-storage-cost.sh` still green on all 124 rows — the change touches only the test's own workload, never a measured row. This is the single failure shared by #3750, #3752 and #3754. It is not stale-base: the tool is byte-identical between `bench/05-gas-walls` and this branch, and it reproduces here. Claude-Session: https://claude.ai/code/session_014mxKpC4vXk9Bujuetq7KBk
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
2 participants
Stack 3 of 5. Base: #3751. Independent of #3751 in content — it touches only
tools/storage-cost/and could be rebased onto #3750 directly if you would rather land it first.Widens the blocking cost snapshot from 24 rows to 48: adds
rga,lww_registerandnested_mapalongside the three collections #3660 landed with.Two findings came out of doing it
1.
RGA::insert(pos, char)is O(n) per call, so building a document one character at a time is quadratic —n + ~47storage rows per character, measured 63.5 / 147.7 / 547.1 / 2,047.0 at n = 10 / 100 / 500 / 2,000. No existingCostShapecould describe that honestly:FlatPerEntryfails for the wrong reason andKnownLinearInNdescribes a point read, not a build. AddsQuadraticBuild, with a ratchet that fails if the curve worsens and, with a different message, if it silently stops being quadratic.2. The tolerance check only failed one way.
reproducible.rsfailed a tolerance that was too tight — a flake generator — while nothing failed one that was gratuitously wide. Three separate comments claimed it caught both. Live consequence:vector_get_nthdeclared 25% against a measured spread of 10.5% at n=10 and under 3% above, so its 13,328 committed rows admitted anything to ~16,660 and a real 20% regression inVector::getpassed silently. The check is now two-sided and the band is 18%, verified by breaking a row and watching the gate catch what the old one let through.Deliberately not registered
sorted_map. WithoutRuntimeEnv::with_indexwired,index_bridge()returnsNoneand everystorage_index_*call falls through to a thread-local mock that never touches the counting closures — so measuring it would attribute zero cost to all index maintenance while printing plausible numbers. The reasoning lives in the registry, not only in a review comment.Verification
flat_curveandreproduciblepass; snapshot regenerated to 48 rows;./scripts/check-storage-cost.shgreen across repeated runs; break-and-revert proof performed against a new row (nested_map_insert), confirming the widened gate bites on the rows this PR adds and names the workload at all four sizes.Note for the reviewer
reproducible.rsandcheck-storage-cost.sharrive byte-identical from #3660; the two-sided rule changes someone else's work. The rule itself (3× worst observed spread + 8 points) is empirically tuned across 11 live measurements rather than derived from first principles — if you want something stricter, this is the line to argue about.