perf(collections): delete the per-probe virtual call on reference-type keys - #322
Merged
Conversation
…e keys The probe loops tested for a vacant slot with `EqualityComparer<TKey>.Default.Equals(slot, default(TKey))`. The JIT devirtualizes that for value-type keys, but not under __Canon-shared reference-type instantiations — so every string-keyed table paid a real interface call per probe iteration to ask whether a reference is null. Route every vacant-slot test through a new internal `EmptySlot.Is<T>` helper whose `typeof(T).IsValueType` guard the JIT folds to a constant: reference-type instantiations compile to a plain null test, value-type ones keep the existing intrinsic comparison. The substitution is exact — the runtime's default comparers resolve a null right-hand side structurally, before consulting the key's own Equals. Also adds the suite's first string-keyed benchmark rows and documents, with numbers, that `CelerityDictionary` measures behind the BCL `Dictionary` on string keys and that `HashCachingDictionary` is the answer there. Closes #308
Coverage
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves probe-loop performance for reference-type keys across Celerity’s open-addressed collections by routing vacant-slot checks through a new EmptySlot.Is<T> helper that compiles to a plain null test under __Canon sharing, while preserving value-type semantics. It also adds cross-collection tests and a new benchmark row to keep this reference-key path tracked and documented.
Changes:
- Introduce
EmptySlot.Is<T>and replaceEqualityComparer<T>.Default.Equals(slot, default)vacant-slot checks across multiple collections. - Add
ReferenceKeyProbeTeststo pin the null-comparer contract and probe correctness for adversarial reference keys. - Add
StringKeyProbeBenchmark+ dashboard entries, and update docs/README/roadmap/changelog to reflect guidance for string-keyed workloads.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/dev/bench/index.html | Adds the “String-keyed probe” card entry to the dev benchmark dashboard. |
| web/dev/bench/detail.html | Adds “String-keyed probe” to the detail page collection list. |
| src/Celerity/Collections/SwissSet.cs | Routes default/empty checks through EmptySlot.Is. |
| src/Celerity/Collections/SwissDictionary.cs | Routes default/empty key checks through EmptySlot.Is. |
| src/Celerity/Collections/RobinHoodSet.cs | Replaces vacant-slot checks with EmptySlot.Is and removes now-unused comparer locals. |
| src/Celerity/Collections/RobinHoodDictionary.cs | Replaces vacant-slot checks with EmptySlot.Is and removes now-unused comparer locals. |
| src/Celerity/Collections/PooledCeleritySet.cs | Replaces vacant-slot checks with EmptySlot.Is and removes now-unused comparer locals. |
| src/Celerity/Collections/PooledCelerityDictionary.cs | Replaces vacant-slot checks with EmptySlot.Is and removes now-unused comparer locals. |
| src/Celerity/Collections/HashCachingSet.cs | Routes default/empty checks through EmptySlot.Is. |
| src/Celerity/Collections/HashCachingDictionary.cs | Routes default/empty key checks through EmptySlot.Is. |
| src/Celerity/Collections/EmptySlot.cs | New internal helper that folds vacant-slot checks to a null test for reference-type instantiations. |
| src/Celerity/Collections/CeleritySet.cs | Replaces vacant-slot checks with EmptySlot.Is and removes now-unused comparer locals. |
| src/Celerity/Collections/CelerityMultiSet.cs | Replaces vacant-slot checks with EmptySlot.Is and removes now-unused comparer locals. |
| src/Celerity/Collections/CelerityMultiMap.cs | Replaces vacant-slot checks with EmptySlot.Is and removes now-unused comparer locals. |
| src/Celerity/Collections/CelerityDictionary.cs | Replaces vacant-slot checks with EmptySlot.Is, removes unused comparer locals, and documents the rationale inline. |
| src/Celerity.Tests/Collections/ReferenceKeyProbeTests.cs | Adds adversarial reference-key tests that pin the null-comparer behavior and cross-collection probe correctness. |
| src/Celerity.Benchmarks/StringKeyProbeBenchmark.cs | Adds first string-keyed probe-path benchmarks (lookup hit/miss, set contains). |
| src/Celerity.Benchmarks/Program.cs | Registers StringKeyProbeBenchmark in the core benchmark suite. |
| ROADMAP.md | Marks the roadmap item as done with rationale and documentation pointer. |
| README.md | Updates the decision table guidance for string keys toward hash-caching variants. |
| docs/performance.md | Adds a “Reference-type keys: cache the hash” section with measured results and dashboard pointer. |
| docs/api/collections.md | Calls out the string-key case and links to the performance guidance. |
| CHANGELOG.md | Adds unreleased entries for the perf change, new benchmark/tests, and updated guidance. |
Copilot review on #322: the [Unreleased] bullets recited the JIT/codegen detail and enumerated every affected type, against CLAUDE.md's rule that entries stay short and user-facing (the release workflow extracts the section verbatim).
Benchmarks11 regressions Highlights
Collections (472)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( |
…slot-devirt # Conflicts: # CHANGELOG.md
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 #308.
What
The probe loops asked "is this slot vacant?" with
EqualityComparer<TKey>.Default.Equals(slot, default(TKey)). For a value-type key the JIT'sEqualityComparer<T>.Defaultintrinsic devirtualizes and inlines that, so it is free. For a reference-type key it does not: the collection JITs as a__Canon-shared body and the call stays a real interface dispatch — onecallvirtper probe iteration, to ask whether a reference isnull.All 66 such call sites across twelve collections now route through a new internal
EmptySlot.Is<T>helper:typeof(T).IsValueTypeis a JIT-time constant (__Canononly ever stands in for a reference type), so exactly one arm is compiled into each instantiation. The codebase already relies on this pattern inHyperLogLog.Hash64.The substitution is exact, not an approximation. Every
EqualityComparer<T>the runtime supplies for a reference type resolves anullright-hand side structurally, before it consults the element's ownEquals— soEquals(x, null)istrueexactly whenxisnull, even for a type whoseEqualsclaims equality with everything. That invariant is now pinned by a test rather than assumed.Hoisted comparer locals that became unused after the substitution were removed; the
comparer.Equals(slot, key)calls (the actual key match) are untouched.Measurements
Local
dotnet run -c Release -- --filter "*StringKeyProbeBenchmark*" --job medium, before vs after on the same machine, Celerity arms only. Run-to-run variance on the "after" side was ±1–2% on the 1k rows.CelerityDictionary_Lookup@1kCelerityDictionary_LookupMissing@1kCelerityDictionary_Lookup@100kCelerityDictionary_LookupMissing@100kCeleritySet_Contains@1kCeleritySet_Contains@100kThe in-cache 1k dictionary rows are where the removed dispatch is the largest fraction of the probe; the 100k rows are memory-bound, so the win is smaller and noisier. No row regressed outside noise. Value-type keys are unaffected by construction — the same expression is compiled.
The
IEqualityProvider<T>follow-up: closed, with the reason#308 asks to either open the
IEqualityProvider<T>issue with numbers attached or close the idea. Closing it. A throwawayHashCachingDictionarycontrol arm (added, measured, removed) settles it:Dictionary<string, int>CelerityDictionarylookup (hit)CelerityDictionarylookup (miss)HashCachingDictionarylookup (hit)HashCachingDictionarylookup (miss)Caching the hash per slot — which changes nothing about the equality dispatch — recovers essentially the whole deficit on the probe-heavy miss path. So the residual reference-type-key cost is dominated by re-hashing the key on every probe, not by the remaining
comparer.Equals. The generic-signature blast radius of anIEqualityProvider<T>axis is not justified by that; the actionable answer for string keys is the hash-caching variants Celerity already ships, and that is now documented instead.Parity rollout
EmptySlot.cs(new internal helper); 66 call sites acrossCelerityDictionary,RobinHoodDictionary,SwissDictionary,HashCachingDictionary,PooledCelerityDictionary,CelerityMultiMap,CeleritySet,RobinHoodSet,SwissSet,HashCachingSet,PooledCeleritySet,CelerityMultiSetReferenceKeyProbeTests.cs— 26 tests. Covers all twelve collections with aNullGreedyKeywhoseEqualsclaims equality withnull(fill past several resizes → lookups → half-delete → survivors), thenull-key/element out-of-band round trip on each, a value-type-key control, and an explicit assertion of theEqualityComparernull contract the whole substitution rests onReferenceKeyProbeTestsis the cross-collection suite for this behaviour and covers the whole family in one fileStringKeyProbeBenchmark.cs(new) — first string-keyed rows in the suite:Lookup,LookupMissing(CelerityDictionary<string, …>vsDictionary<string, int>) andContains(CeleritySet<string, …>vsHashSet<string>), at 1k / 100k, library default load factor. Registered inProgram.csCoreBenchmarksweb/dev/bench/index.htmlandweb/dev/bench/detail.htmlgain aStringKeyProbeentry.web/index.htmlship cards: N/A — no new public type shipsdocs/performance.md§1 gains Reference-type keys: cache the hash with the measured table;docs/api/collections.mdHashCachingDictionarysection calls out the string-key case;README.mddecision table points string-keyed workloads atHashCachingDictionary[Unreleased]— bullets for the benchmark, the tests, the perf change, and the documentation correctiondone, including whyIEqualityProvider<T>was not openedTest plan
dotnet buildclean, 0 errors, no new warningsdotnet testgreen on net8.0 / net9.0 / net10.0 — 4,736 tests (4,710 pre-existing, all untouched, plus 26 new). The existing suite passing unchanged is the primary evidence that behaviour is identicalCelerity.Ring31,Celerity.Sentinel20,Celerity.Cardinality23)StringKeyProbeBenchmarkruns end to end locallyEmptySlot.Ishas both arms exercised (value-type keys via the integer collections, reference-type via the new tests)mainmerge, rendering the new String-keyed probe card🤖 Generated with Claude Code