Optimize flat-state snapshot compaction and harden SortedMergeDictionary builds - #12864
Optimize flat-state snapshot compaction and harden SortedMergeDictionary builds#12864benaadams wants to merge 14 commits into
Conversation
…amps Bucket slots now store entryIndex + 1 + salt; the salt grows every build, so slots from earlier builds decode out of range and read as empty. This removes the O(bucketCount) Array.Clear previously paid twice per pooled reuse cycle (NoResizeClear + BuildBuckets). Bucket arrays are zeroed only on rent ([0, size) plus a cleared watermark, as Rent only guarantees length >= size) and on salt overflow. Pooled Entry arrays are kept all-default: the Entry type is private to the class, so its pool only sees this class's returns, which clear exactly the written prefix (_entriesDirty); rents skip clearing entirely (previously full-capacity clears on both rent and return). TryGetValue caches _entries and bounds the chain walk on entries.Length, removing the per-hop field reload and bounds check (JitAsm-verified); the BuildBuckets fill loop gets the same span-based shape. Add regression tests for stale-key resurrection across reuse cycles and rebuild-without-clear.
Zero _count at the start of both build methods so a build that throws (comparer or keep delegate) leaves the dictionary empty instead of a mixed-generation state where lookups could return stale keys or cycle; this also fixes a pre-existing hang when a torn unsorted build left Next = 0 self-loops. Cap the TryGetValue chain walk at count hops. A valid chain descends, so the cap never binds on settled state (the descending-chain property is now load-bearing for lookup results, not just liveness); a reader that races a rebuild in violation of the lease now misses instead of spinning forever on a swapped-out, pool-cleared entry array. Clamp build loops to the counted capacity. ConcurrentDictionary-backed snapshot sources can yield more or fewer items than Count if mutated; other source types require the same quiescence lease and may instead fail through enumerator invalidation. More items would write live refs past the _entriesDirty watermark and return them to the pool uncleared; fewer would sort unwritten default entries into the build. Add tests for the throwing-keep torn build, Count/enumeration mismatches in both directions, and full-length single-bucket chains (hop-cap boundary).
Snapshot compaction requires quiescent sources. Throw on both over- and under-yield instead of silently truncating or sorting unwritten entries, aligning ConcurrentDictionary-backed inputs with the fail-loud behavior of the plain Dictionary sources. Keep the exception construction and messages in StackTraceHidden, DoesNotReturn helpers so the build hot path carries neither throw construction nor cold string loads.
Replace the floating-point 0.7 load-factor calculation with count * 10L / 7 before power-of-two rounding. An exhaustive sweep over all 2^31 non-negative counts found zero bucket-size mismatches with the previous double expression. The proposed ceil form diverged at 23 counts (first at count=1), while long arithmetic also avoids int overflow above roughly 214 million entries. The constant integer division lowers to multiply-shift instead of the previous floating-point conversion and divide sequence.
Mutable snapshot inputs are sorted into owned Run buffers without building lookup buckets that MergeInto never reads. A Run exposes only entries and count, so a bucketless transient cannot accidentally be queried; its owner still clears reference-bearing entries before returning the buffer. The pre-implementation benchmark at c5ab148da7 measured the run-only build about 1.3x faster for a 1 million-entry transient and the combined build-plus-merge path about 1.10-1.13x faster. Bucket stamping, bucket-array rent/clear work, and salt advancement were therefore removed from this transient-only path.
Copy the sole non-empty run directly and merge two runs with two cursors, preserving source priority and keep filtering without constructing a loser tree. The compaction schedule is a ruler sequence, so k=2 is the modal compaction size. The focused pre-implementation benchmark measured the two-cursor k=2 path about 2.6x faster, with k=1 near-free and k>=8 neutral. A final BenchmarkDotNet 0.15.8 ShortRun cross-check on .NET 10.0.11 (SDK 10.0.400), Windows 11, Ryzen 9 9950X used the full sorted payload with 16 self-destructs. At k=2, merge base / salt-only / hardening / final measured 7.151 / 7.615 / 7.930 / 3.334 ms, a 2.1-2.4x end-to-end final-branch improvement. At k=32 the same points were 254.35 / 329.10 / 314.85 / 262.57 ms, but 16-48 ms standard deviations on the first three optimized points make that comparison noise-bound; it supports no k=32 win or regression claim.
BenchmarkDotNet 0.15.8 ShortRun on .NET 10.0.11, Windows 11, Ryzen 9 9950X. Each method included the required position clear and tree sentinel initialization. Fresh arrays allocated two int[k] arrays; the pool benchmark rented and returned two arrays, making its result conservative versus the single combined pooled buffer used by the implementation. Representative means: - k=64: stack 9.53 ns, pool 27.04 ns, fresh 41.20 ns / 560 B - k=128: stack 16.98 ns, pool 26.25 ns, fresh 61.97 ns / 1,072 B - k=256: stack 34.40 ns, pool 28.50 ns, fresh 120.52 ns / 2,096 B - k=2048: stack 279.70 ns, pool 98.29 ns, fresh 1,196.45 ns / 16,432 B The measured crossover is between k=128 and k=256, so use a fixed 256-int stack buffer through 128 sources and one ArrayPool buffer above that threshold. SkipLocalsInit avoids clearing the fixed stack buffer; LoserTree initializes the tree and position slices it consumes.
Replace the per-entry boundary dictionary closures with a generic, stateful keep policy. Storage and storage-node keys are ordered by their owning group, so each merge now probes the boundary dictionary once per contiguous group while retaining the existing Func path for compatibility. BenchmarkDotNet 0.15.8 ShortRun on .NET 10.0.11 (SDK 10.0.400), Windows 11, Ryzen 9 9950X compared this cached policy with a pooled uint bit-mask pre-pass that also cached group boundaries while scanning each source. At the full benchmark payload with 32 sorted snapshots and 16 self-destructs, cached measured 231.10 ms and the mask 218.87 ms (one run; mask 5.3% faster). At the production-scale source count with a reduced payload (2048 sorted snapshots; 16 accounts, 4 contracts x 8 slots, 32 state nodes, and 4 x 32 storage nodes per snapshot), the result reversed and replicated: cached 79.43/81.80 ms versus mask 86.32/85.18 ms, making cached 4.0-8.0% faster per paired run. Managed allocation was identical (577.56 KB/op). The large-k bandwidth gate therefore selects the zero-mask cached policy.
|
Claude finished @benaadams's task in 9m 55s —— View job Review: Optimize flat-state snapshot compaction and harden
|
| # | Finding | Where |
|---|---|---|
| 1 | Pooled loser-tree scratch isn't returned when the comparer or keep filter throws — Return is on the normal path, not in a finally |
SortedMergeDictionary.cs:227-240 |
| 2 | _count is published before BuildBuckets(), so an OOM inside BuildBuckets leaves a non-zero count against previous-build bias/stamps — the one gap in the fail-closed invariant |
SortedMergeDictionary.cs:243-245 |
| 3 | Run's distinct-key precondition is now load-bearing (the k≤2 paths don't dedupe within a run; the loser tree did) but undocumented |
SortedMergeDictionary.cs:47-51 |
| 4 | StorageBoundaryKeep missing the <remarks> that explains why the memoization is safe |
SnapshotCompactor.cs:228 |
| 5 | Pre-existing task-join race in CompactSnapshotBundle, described below |
SnapshotCompactor.cs:162-170 |
#5 in detail (GitHub wouldn't take an inline comment — those lines aren't in the diff): if content.SortedSelfDestructs.BuildFromUnsorted(...) on line 162 throws, Task.WaitAll is never reached, so the catch returns content to the resource pool and the using PooledDictionary declarations dispose slotClearBoundary/nodeClearBoundary — all while the four Task.Run merges are still writing into content and still calling boundaries.TryGetValue through StorageBoundaryKeep. Another thread can then GetSortedSnapshotContent and receive an object under concurrent mutation.
This predates the PR (the old slotKeep/nodeKeep lambdas captured the same dictionaries), and the InvalidOperationExceptions this PR adds aren't reachable from selfDestructMerged — a PooledDictionary's Count always matches its enumeration — so the trigger stays OOM-class. Raising it because the method is being touched and the fix is a one-liner: move Task.WaitAll(compactTask.AsSpan()) into a finally so the tasks are always joined before anything they touch is released.
Smaller notes, no action needed
Entrywidened fromprivatetointernal. That makesArrayPool<SortedMergeDictionary<K,V>.Entry>.Sharedrentable from anywhere inNethermind.State.Flat, so the "returned arrays are all-default" convention now has a wider blast radius than whenEntrywas private. The class<remarks>does call this out, which is the right mitigation.- The public
BuildFromMerge(ReadOnlySpan<SortedMergeDictionary<…>>, …)overload allocatesnew Run[sources.Length]per call. Only tests and theMergehelper reach it now — production goes through theRunoverload — so it's off the hot path. - Setting
_count = 0at the top of the public overload means a self-merge (thisamongsources) now silently sees itself as empty rather than producing garbage. Neither was ever supported; just noting the behavior changed. - The safety argument for
TryGetValuenow spans two fields that must agree (_bucketscontents ↔_bucketBias), where the old 1-based encoding was self-describing. Under a torn read theKey.Equalscheck still prevents returning a wrong value for a key, so the worst case is a stale value or a miss — same class as before, and the quiescent-source contract covers it. The comment at:97-99documents this well.
Repo-rule adherence
Clean. No LINQ on the hot paths, no var, is null throughout, [DoesNotReturn, StackTraceHidden] throw helpers keep the exception construction off the hot path, no #region, comments explain why. The IMergeKeep<TKey> generic-struct approach adds an internal interface but no public surface, and it's the mechanism that gets the keep filter devirtualized — a reasonable trade against the "don't add interfaces" guidance. MergeInto's Func-based overload preserving the old call shape keeps the diff additive.
What I could not do
Build and test execution were not permitted in this environment (dotnet build requires approval), so I did not independently reproduce the 110/110 focused run or dotnet format whitespace. All findings above are from static analysis. Adding Bash(dotnet build:*) and Bash(dotnet test:*) to --allowedTools would let me verify next time.
· branch sortedmergedictionary
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the flat-state snapshot compaction merge pipeline by reducing per-build bucket clearing work, adding fast paths for common merge sizes, and hardening SortedMergeDictionary against torn/failed builds (fail-closed behavior and bounded lookup walks).
Changes:
- Introduces
Run/PooledRunviews and typed keep policies (IMergeKeep) to avoid unnecessary bucket work for transient merge inputs and reduce delegate overhead. - Reworks bucket handling to use salted stamps (no per-build
Array.Clearof buckets) and adds hop-cappedTryGetValueto prevent infinite loops on torn state. - Updates snapshot compaction to use a cached boundary keep policy for storage/storage-node merges and expands tests to cover new failure/fast-path behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Nethermind/Nethermind.State.Flat/SnapshotCompactor.cs | Switches merge keep logic to typed keep policies and uses bucketless transient runs for mutable snapshot sources. |
| src/Nethermind/Nethermind.State.Flat/Collections/SortedMergeDictionary.cs | Adds run abstractions, salted bucket stamps, fast merge paths, hop-capped lookups, and new hardening checks for build inputs. |
| src/Nethermind/Nethermind.State.Flat.Test/Collections/SortedMergeDictionaryTests.cs | Adds/updates tests for reuse cycles, torn builds, count mismatches, hop-cap boundaries, and run-vs-dictionary merge equivalence. |
Suppressed comments (1)
src/Nethermind/Nethermind.State.Flat/Collections/SortedMergeDictionary.cs:364
- When renting a new Entry[] from ArrayPool in EnsureEntryCapacity, the array is not cleared. For reference-containing TKey/TValue instantiations this can retain foreign object references in the rented buffer (especially beyond the written prefix) for the lifetime of the dictionary, and it also breaks the class remark that pooled entry arrays are returned all-default. Clearing the rented array (conditionally on IsReferenceOrContainsReferences) when the backing buffer changes restores the invariant and avoids unintended memory retention.
if (entries.Length < count)
{
_entries = ArrayPool<Entry>.Shared.Rent(count);
if (entries.Length > 0)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
Publish Count only after bucket construction succeeds so a failed build remains empty. Document the sorted/distinct Run precondition and cover preservation of an existing dirty watermark across successful and aborted filtered merges. Tests: 111 SortedMergeDictionary and SnapshotCompactor tests passed.
Join every successfully scheduled merge task from a finally block before pooled snapshot content and boundary dictionaries can be returned. This prevents workers from continuing against resources that another compaction may already have reused when the main-thread build fails. Tests: 111 SortedMergeDictionary and SnapshotCompactor tests passed.
|
Addressed the remaining actionable review notes in two focused commits:
Focused |
|
@claude re-review |
|
Claude finished @benaadams's task in 5m 3s —— View job Re-review: Optimize flat-state snapshot compaction and harden
|
Remove the catch used solely to return a direct ArrayPool<Entry>.Shared rental when transient run construction aborts. Count mismatches detected by this code still clear and return the rental before the cold throw helper; unexpected enumeration, hashing, and comparer failures unwind without an exception region and leave the array for GC reclamation. Tests: 111 SortedMergeDictionary and SnapshotCompactor tests passed.
Record that unexpected build failures intentionally abandon a direct ArrayPool<Entry>.Shared rental, while deliberate count-mismatch failures return it before throwing. This prevents future cleanup changes from reintroducing an exception region solely for Return. Tests: not run (comment-only change).
|
@claude re-review |
|
Claude finished @benaadams's task in 2m 34s —— View job Re-review: Optimize flat-state snapshot compaction and harden
|
| # | Severity | Finding | Where |
|---|---|---|---|
| 1 | Low | The deliberate count-mismatch path on BuildRunFromUnsorted is untested — only the instance BuildFromUnsorted is covered, and the run variant now has its own copy of clear-then-return-then-throw. Its bug mode is silent (a non-default array back in the shared pool, surfacing later as a stale key in an unrelated build). |
SortedMergeDictionary.cs:139-152 |
On the pool-return convention (#12865)
For what it's worth, the reasoning in #12865 is sound and I'd rather review against it than against my earlier instinct: ArrayPool Return is a reuse optimization, so its absence on an aborting path costs one reuse, whereas Dispose on a real resource is a correctness obligation. The line that keeps this from being a blanket "never bother" is the one the rule draws — reference-bearing buffers must still be cleared when they are returned, and finally blocks that already exist for another reason should still return. Both hold in this file.
Nothing new in the fresh pass
Re-confirmed against the current HEAD: salt arithmetic and the overflow reset (:404-418) are unchanged and still can't produce a stale in-range decode; BuildBuckets(count) → _count = count ordering holds on both build paths; MergeInto's finally still disposes every transient that was successfully constructed, and a BuildRunFromUnsorted that throws mid-loop simply abandons its own rental while the earlier transients are disposed normally; StorageBoundaryKeep unchanged.
What I could not do
Build and test execution are still not permitted here — dotnet build came back as requiring approval — so I did not reproduce the 111/111 focused run or dotnet format whitespace. Everything above is static analysis. Adding Bash(dotnet build:*) and Bash(dotnet test:*) to --allowedTools would let me verify next time.
· branch sortedmergedictionary
Changes
SortedMergeDictionaryis the pooled sorted-array-plus-hash-bucket structure that flat-state snapshot compaction (SnapshotCompactor) builds from snapshot sources and merges into compacted snapshots. This PR removes measured dead work and per-entry overhead from the build and merge paths, and turns silent failure modes into loud ones.Correctness and hardening
_countis zeroed at the start of every build, so a throwing comparer, keep filter, or source enumerator leaves the dictionary empty and reusable instead of in a mixed-generation state where lookups could return stale keys. This also fixes a pre-existing lookup hang when a torn unsorted build leftNext = 0self-loops.TryGetValuechain walks are capped at the logical count. A valid chain descends, so the cap never binds on settled state; a reader racing a rebuild (in violation of the quiescent-source contract) now misses instead of spinning forever on a swapped-out, pool-cleared entry array.Countnow throw instead of being silently truncated or padded with unwritten sorted entries. Snapshot compaction requires quiescent sources; a mismatch means that contract was broken, and for consensus-relevant flat state a loud compaction abort is strictly better than silently dropping entries. Exception construction sits in[DoesNotReturn, StackTraceHidden]helpers off the hot path.Build and merge performance
entryIndex + 1 + saltand the salt advances every build, so slots written by earlier builds decode out of range and read as empty. This removes theO(bucketCount)Array.Clearpreviously paid twice per pooled reuse cycle. PooledEntryarrays now clear exactly the written prefix (_entriesDirty) on return and skip clearing on rent.MergeIntoconsumes them only through entries and count, so bucket stamping, bucket-array rent/clear, and salt advancement were dead work on that path. The internalRunview exposes noTryGetValueby construction, so a bucketless transient cannot be queried by accident; its pooled owner still clears reference-bearing entries before returning the buffer.new int[k]scratch arrays become one adaptive buffer: a fixed 256-int[SkipLocalsInit]stack buffer through 128 sources and a single pooled buffer above that, with the threshold set by the measured crossover.count / 0.7tocount * 10L / 7before power-of-two rounding. An exhaustive sweep over all 2^31 non-negative counts found zero bucket-size mismatches with the previous double expression; the constant division lowers to multiply-shift, and the long arithmetic avoids int overflow above roughly 214M entries.StorageBoundaryKeep<TGroup, TSecondary>) probes the self-destruct boundary dictionary once per contiguous group instead of once per merged entry. The existingFunc-based keep path remains for compatibility and tests.Measured results
All benchmarks: BenchmarkDotNet 0.15.8 ShortRun, .NET 10.0.11 (SDK 10.0.400), Windows 11, Ryzen 9 9950X.
Code generation was verified with disassembly:
TryGetValuecompiles to 135 bytes FullOpts with the hop cap as three register-only instructions (dec eax / test eax,eax / jle),entries.Lengthcached in a register, and a single range check covering only the bucket index. The integer bucket sizing contains no floating-point operations (lea, magic-constant multiply/shift,lzcnt/shrx).Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
New tests cover: stale-key resurrection across pooled reuse cycles, rebuild-without-clear, torn builds from a throwing comparer/source enumerator/keep filter (dictionary left empty and reusable), Count/enumeration mismatch in both directions, salt-overflow reset, full-length single-bucket chains at the hop-cap boundary, one- and two-source fast-path semantics (duplicate-key priority, fully filtered keys, empty sources, original source indexes), and randomized merge equivalence that also exercises the pooled loser-tree path at 256 sources.
Focused runs pass 110/110 (
SortedMergeDictionaryTests+SnapshotCompactorTests). The fullNethermind.State.Flat.Testproject passes 971, skips 10, and fails 14; every failure is the pre-existing Windows teardown file-lock issue on arena files (ArenaManager/StorageLayer/LongFinality), unrelated to this change.dotnet format whitespacereports no changes.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
Invariants preserved by every change: key-sorted enumeration order (persistence depends on it), keep-filter semantics (highest-index source wins on equal keys; fully filtered keys omitted), pool hygiene (returned arrays are all-default), and torn-build atomicity.
The k=32 comparison is reported as noise-bound rather than claimed as a win. Further candidates (64-bit sort key prefixes, batched equal-key handling in the loser tree, SoA entry layout) are deliberately left out: they are unmeasured or change
Entrylayout, and none block this change.