Repository navigation
perf: stop retaining ConsistentHash ring's SortedDictionary (#8293) - #8324
Merged
Aaronontheweb merged 9 commits intoAug 26, 2026
Merged
Aaronontheweb merged 9 commits into
Aaronontheweb merged 9 commits into
Conversation
…et#8293) ConsistentHash<T> kept the ring in both a SortedDictionary and the parallel int[]/T[] arrays that back NodeFor's binary search. Once the arrays exist the dictionary is dead weight. Materialize the arrays once in the constructor and drop the dictionary (Option A from akkadotnet#8293); ConsistentHash.Create and its 32-bit collision handling are unchanged, so the ring stays byte-identical. Reclaims the retained SortedDictionary (~1.07 MB at 20k ring points, ~2.67 MB at 50k) for a long-lived ring. Also removes an unsynchronized lazy array init (a torn read of the nullable tuple under concurrent Select) and a per-message enumerator allocation in IsEmpty (_nodes.Any()) on net48/netstandard2.0. Adds constructor snapshot and null-guard tests; records the snapshot behavior change in BREAKING_CHANGES_V1.6.md. Enables #nullable on both touched files.
orange-dot
force-pushed
the
feature/8293-consistenthash-upstream
branch
from
July 5, 2026 16:16
c0c9285 to
b7ebe0c
Compare
Aaronontheweb
approved these changes
Aug 26, 2026
| // </copyright> | ||
| //----------------------------------------------------------------------- | ||
|
|
||
| #nullable enable |
Aaronontheweb
enabled auto-merge (squash)
August 26, 2026 21:47
This was referenced Oct 1, 2026
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
Oct 2, 2026
…et#8293) (akkadotnet#8324) ConsistentHash<T> kept the ring in both a SortedDictionary and the parallel int[]/T[] arrays that back NodeFor's binary search. Once the arrays exist the dictionary is dead weight. Materialize the arrays once in the constructor and drop the dictionary (Option A from akkadotnet#8293); ConsistentHash.Create and its 32-bit collision handling are unchanged, so the ring stays byte-identical. Reclaims the retained SortedDictionary (~1.07 MB at 20k ring points, ~2.67 MB at 50k) for a long-lived ring. Also removes an unsynchronized lazy array init (a torn read of the nullable tuple under concurrent Select) and a per-message enumerator allocation in IsEmpty (_nodes.Any()) on net48/netstandard2.0. Adds constructor snapshot and null-guard tests; records the snapshot behavior change in BREAKING_CHANGES_V1.6.md. Enables #nullable on both touched files. Co-authored-by: Aaron Stannard <aaron@petabridge.com> (cherry picked from commit 60a01d0)
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
Oct 3, 2026
…et#8293) (akkadotnet#8324) ConsistentHash<T> kept the ring in both a SortedDictionary and the parallel int[]/T[] arrays that back NodeFor's binary search. Once the arrays exist the dictionary is dead weight. Materialize the arrays once in the constructor and drop the dictionary (Option A from akkadotnet#8293); ConsistentHash.Create and its 32-bit collision handling are unchanged, so the ring stays byte-identical. Reclaims the retained SortedDictionary (~1.07 MB at 20k ring points, ~2.67 MB at 50k) for a long-lived ring. Also removes an unsynchronized lazy array init (a torn read of the nullable tuple under concurrent Select) and a per-message enumerator allocation in IsEmpty (_nodes.Any()) on net48/netstandard2.0. Adds constructor snapshot and null-guard tests; records the snapshot behavior change in BREAKING_CHANGES_V1.6.md. Enables #nullable on both touched files. Co-authored-by: Aaron Stannard <aaron@petabridge.com> (cherry picked from commit 60a01d0)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8293.
What
ConsistentHash<T>kept its ring in two structures at once: theSortedDictionary<int, T>it was built from and the parallelint[]/T[]arrays it lazily materialized to backArray.BinarySearchinNodeFor. Once the arrays exist the dictionary is dead weight — a live ring is served entirely by the arrays.This is Option A from the design in akkadotnet/akka.net#8293 (the option the issue recommends): stop retaining the
SortedDictionary. The ring is now materialized into the two sorted arrays once, in the constructor, and the dictionary is dropped.ConsistentHash.Create— the incremental tree build and its 32-bit collision probing (#8031 / #8294) — is completely unchanged, so the ring is byte-identical to before.Why this is safe (review-my-own-PR)
The ring is byte-identical in the no-collision path.
Createstill builds the sameSortedDictionarythe same way; the constructor just readsKeys/Valuesout of it instead of holding onto it. The executable before/after proofConsistentHashSpec.Create_must_produce_the_legacy_ring_whenever_the_legacy_algorithm_succeedsstill passes, which is what keeps rolling upgrades safe.Readers redirected off the dictionary, all onto the arrays:
NodeForalready read the arrays — no change.IsEmptynow reads_nodeHashRing.Length == 0instead of_nodes.Any().operator +/operator -already rebuilt viaCreate; they now feed it the values array (Distinct()collapses thevirtualNodesFactorrepeats) instead of_nodes.Values.Public API preserved (extend-only). The
ConsistentHash(SortedDictionary<int,T>, int)constructor stays; it simply materializes the arrays from the passed dictionary and no longer retains it.Akka.API.Testsapprovals are unchanged.Two bug-adjacent improvements this drops out for free
_ring ??= (...)wrote a multi-word(int[], T[])?with no synchronization.ConsistentHashingRoutingLogic.Selectshares one ring across threads via anAtomicReference, so two threads racing the firstNodeForon a fresh ring could observe a torn read of that nullable tuple (has-value visible before the array fields) — an NRE on weak memory models (ARM64). Materializing the arrays in the constructor asreadonlyfields removes the race.IsEmptyran_nodes.Any(), andNodeForcallsIsEmptyon every routed message;Enumerable.Any()boxes an enumerator each call._nodeHashRing.Length == 0is a field read.Footprint (the point of the change)
Retained heap of a live ring (
factor = 10), measured by rooting rings and readingGC.GetTotalMemory(true)before/after — the steady-state cost a router pays for the ring's whole lifetime:(Each figure is the mean over 20 rooted rings, isolated per process,
GC.GetTotalMemory(true)before/after; i7-4600U, .NET 10, workstation GC. The "after" figures match the bare array cost —int[]+T[]— confirming nothing else is retained. Lines up with the issue's estimate of ~1.35 MB → ~0.24 MB and ~3.4 MB → ~0.6 MB.)BenchmarkDotNet,
ConsistentHashCreateBenchmarks(Createruns once per membership change, off the message path;NodeForis the per-message hot path, included to confirm no regression):NodeFor(per-message hot path), before → after — flat within noise, no regression:Create(once per membership change, off the message path), before → after:Both
CreateMeanandAllocatedrise. The lookup arrays used to be built lazily on the firstNodeFor; theCreatebenchmark returns a ring it never looks up in, so before this change it never paid for materializing them. They are now built eagerly in the constructor, so thatToArraycost (both the CPU and the ~600 KB at 50k points — note theAllocateddelta matches the array size) moves intoCreate. In production the arrays were always going to be built on the first lookup anyway, so the total Create + first-lookup work is essentially unchanged — only its timing shifts earlier, onto a path that runs once per membership change. BDN's per-callAllocatedcannot see the retained-footprint win (the table above), which is the whole point of the change — hence the separate measurement. (Numbers on a 2-core i7-4600U under load)Behavioral note
Because the ring is now snapshotted in the constructor, mutating the
SortedDictionaryafter construction no longer affects the instance. Before this change the aliasing was inconsistent anyway —IsEmptyand the operators read the dictionary live, whileNodeForfroze it after the first lookup. Anulldictionary now throwsArgumentNullExceptionfrom the constructor instead of surfacing later as anNullReferenceException.ConsistentHash.Createalready builds the dictionary fully before constructing, so no routing code is affected. Recorded inBREAKING_CHANGES_V1.6.md.Tests
Existing
ConsistentHashSpec(collision handling, ring identity, legacy-ring equality proof) is unchanged and green. Added:Constructor_must_snapshot_the_dictionary_and_not_retain_it— pins the non-retention: build a dictionary, construct, clear the dictionary, assert the ring still routes and is unchanged.Constructor_must_throw_ArgumentNullException_for_a_null_dictionary— pins the new null guard.#nullable enablewas added to both touched files per the contributor guidelines.