feat(collections): span-keyed lookups on the string-keyed collections + StringInternTable - #323
Merged
Merged
Conversation
… + StringInternTable Adds ISpanHashProvider to Celerity.Hashing and, on top of it, allocation-free ReadOnlySpan<char> lookups across every string-keyed collection, plus the type that pattern makes possible: StringInternTable. Before this, a parser holding a slice of its input buffer had to call new string(span) to probe any Celerity string-keyed type — one allocation plus a copy per lookup, on the hot path of exactly the workloads those types exist for. .NET 9 closed that gap for the BCL Dictionary with GetAlternateLookup; net8.0 is this library's floor, so it had no answer at all. - ISpanHashProvider (int Hash(ReadOnlySpan<char>)) is a sibling interface, not an overload on IHashProvider<T>: a ref struct could not be a generic type argument before allows ref struct, so the span has to be a method parameter. All 23 String*Hasher types implement it, each sharing one body between the two overloads so they cannot drift. - Span TryGetValue / ContainsKey / Contains on FrozenCelerityDictionary, FrozenCeleritySet, CelerityDictionary<string, ...>, CeleritySet<string, ...> and Trie<TValue>. On the four hashed types these are extension methods carrying the extra constraint on the method, so no shipped type's own constraints change and the JIT still devirtualizes the hash call. - StringInternTable / StringInternTable<THasher>: GetOrAdd(ReadOnlySpan<char>) returns the canonical string and allocates only on a miss. Full parity rollout in this commit: dedicated + cross-collection tests, a hasher parity suite, a CsCheck differential, a Celerity.Fuzz target, an AOT smoke-test block, a tracked benchmark plus SpanLookup rows on two existing ones, dashboard wiring, API reference and README sections, CHANGELOG, ROADMAP. Closes #311 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a span-keyed lookup “lane” for string-keyed collections (net8-compatible) by introducing ISpanHashProvider, implementing it across the built-in String*Hasher family, and exposing span lookup APIs (mostly via extension methods) plus a new StringInternTable for span-keyed string interning.
Changes:
- Introduce
ISpanHashProviderand implement span hashing across all built-inString*Hashertypes, with parity tests enforcingHash(string) == Hash(ReadOnlySpan<char>). - Add allocation-free span-keyed lookups for
FrozenCelerityDictionary/Set,CelerityDictionary<string,...>,CeleritySet<string,...>, andTrie<TValue>. - Add
StringInternTable(+ tests, fuzz target, benchmarks, and docs/dashboard wiring) to intern strings from spans with allocation only on misses.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/index.html | Adds StringInternTable to the “What ships in the box” list. |
| web/dev/bench/index.html | Adds dashboard wiring for StringInternTable and SpanLookup ops. |
| web/dev/bench/detail.html | Adds StringInternTable to benchmark detail list. |
| src/Celerity/Collections/Trie.cs | Adds span overloads and generalizes descent to ReadOnlySpan<char>. |
| src/Celerity/Collections/StringInternTable.cs | New span-probed interning table that allocates only on misses. |
| src/Celerity/Collections/SpanLookupExtensions.cs | New extension-method surface for span-keyed lookups on hashed collections. |
| src/Celerity/Collections/FrozenCeleritySet.cs | Adds internal span-slot probing + internal hasher accessor for extensions. |
| src/Celerity/Collections/FrozenCelerityDictionary.cs | Adds internal span-slot probing, value accessor, and internal hasher accessor. |
| src/Celerity/Collections/CeleritySet.cs | Adds internal span probing hook used by span lookup extensions. |
| src/Celerity/Collections/CelerityDictionary.cs | Adds internal span probing hook + value/hasher accessors used by extensions. |
| src/Celerity.Tests/Hashing/SpanHashParityTests.cs | New contract tests for ISpanHashProvider roster + parity assertions. |
| src/Celerity.Tests/Collections/StringInternTableTests.cs | Dedicated unit tests for StringInternTable. |
| src/Celerity.Tests/Collections/StringInternTableDifferentialTests.cs | Seeded differential tests vs a Dictionary<string,string> oracle. |
| src/Celerity.Tests/Collections/SpanLookupTests.cs | Cross-collection tests asserting span lookups match string lookups. |
| src/Celerity.Hashing/StringXxHash64Hasher.cs | Implements ISpanHashProvider; routes string hashing through span core. |
| src/Celerity.Hashing/StringXxHash3Hasher.cs | Implements ISpanHashProvider; routes string hashing through span core. |
| src/Celerity.Hashing/StringXxHash32Hasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringSipHash24Hasher.cs | Implements ISpanHashProvider; unifies core via span. |
| src/Celerity.Hashing/StringSipHash13Hasher.cs | Implements ISpanHashProvider; unifies core via span. |
| src/Celerity.Hashing/StringSdbmHasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringMurmur3Hasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringMurmur2Hasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringMetroHash64Hasher.cs | Implements ISpanHashProvider; unifies core via span. |
| src/Celerity.Hashing/StringJenkinsOaatHasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringHighwayHash64Hasher.cs | Implements ISpanHashProvider; unifies core via span. |
| src/Celerity.Hashing/StringHalfSipHash24Hasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringFnV1Hasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringFnV1AHasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringFnV1AFullHasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringFnV1A64Hasher.cs | Implements ISpanHashProvider; unifies core via span. |
| src/Celerity.Hashing/StringFnV164Hasher.cs | Implements ISpanHashProvider; unifies core via span. |
| src/Celerity.Hashing/StringElfHasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringDjb2Hasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringDjb2AHasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringCrc32Hasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/StringCityHash64Hasher.cs | Implements ISpanHashProvider; unifies core via span. |
| src/Celerity.Hashing/StringAdler32Hasher.cs | Implements ISpanHashProvider with span overload. |
| src/Celerity.Hashing/ISpanHashProvider.cs | New interface defining span hashing contract. |
| src/Celerity.Fuzz/Differential.cs | Adds StringInternTable differential fuzz target. |
| src/Celerity.Benchmarks/TrieBenchmark.cs | Adds SpanLookup benchmark category. |
| src/Celerity.Benchmarks/StringInternTableBenchmark.cs | New benchmark for StringInternTable vs BCL shapes. |
| src/Celerity.Benchmarks/Program.cs | Registers StringInternTableBenchmark in the suite. |
| src/Celerity.Benchmarks/FrozenCelerityDictionaryBenchmark.cs | Adds SpanLookup benchmark category. |
| src/Celerity.AotSmokeTest/Program.cs | Exercises new span APIs + intern table under AOT. |
| ROADMAP.md | Marks the span-keyed lookup roadmap item as done (with rationale). |
| README.md | Documents span lookups and StringInternTable in user-facing docs. |
| docs/api/hashing.md | Adds ISpanHashProvider section and contract guidance. |
| docs/api/collections.md | Adds span-keyed lookup documentation + StringInternTable docs. |
| CHANGELOG.md | Adds [Unreleased] entries for the new features/tests/bench/docs. |
…BCL baseline per benchmark category Addresses the Copilot review on #323. The span probes reinterpret each slot as a string via Unsafe.As, which is a no-op only while the collection is closed over string. The extension methods' signatures are what guarantee that, and nothing in the type system restates it inside the probe — so a future in-assembly caller could silently get undefined behaviour. Debug.Assert is [Conditional("DEBUG")], so the guardrail costs nothing in a release build and the hot path is unchanged. Separately, self-review of the dashboard wiring: StringInternTableBenchmark had two BCL arms in its Dedupe category (HashSet and Dictionary). The dashboard indexes one bcl value per collection|op|itemCount, so the two baselines would have overwritten each other and the card would have compared against whichever landed last. Dropped the HashSet arm — Dictionary is also the truer analogue, since it hands back the canonical instance exactly as GetOrAdd does. The Lookup category keeps HashSet<string> as its only BCL arm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
CHANGELOG.md:15
- Same note as above: these bullets read more like PR body text than changelog entries. The repo’s release process uses the version section verbatim as the GitHub Release body, so keeping bullets terse helps avoid hitting size limits (CONTRIBUTING.md:118-124).
- **`SpanHashParityTests`** — a family-wide roster test plus a per-hasher `Hash(s) == Hash(s.AsSpan())` assertion over every length class, checked both as a whole string and as a slice of a larger buffer, since a divergence there would silently report a stored key as absent. Closes [#311](https://github.com/marius-bughiu/Celerity/issues/311).
- **`SpanLookupTests`, `StringInternTableTests`, `StringInternTableDifferentialTests`**, and a `StringInternTable` target in the `Celerity.Fuzz` differential harness — cross-collection coverage asserting each span path against its own type's string overload, plus seeded differential runs against a `Dictionary<string, string>` oracle. `Celerity.AotSmokeTest` exercises every new span instantiation and the intern table under a Native AOT publish. Closes [#311](https://github.com/marius-bughiu/Celerity/issues/311).
- **`StringInternTableBenchmark`** (registered in the CI-tracked suite) plus `SpanLookup` rows on `FrozenCelerityDictionaryBenchmark` and `TrieBenchmark`, with the matching dashboard cards. Allocation is the headline: the BCL arms allocate one string per occurrence, the Celerity arms one per distinct token. Closes [#311](https://github.com/marius-bughiu/Celerity/issues/311).
- API reference sections for `ISpanHashProvider`, span-keyed lookups, and `StringInternTable`, plus README entries in the collections list, the "Choosing a collection" decision table, and the hasher-picking notes. Closes [#311](https://github.com/marius-bughiu/Celerity/issues/311).
Addresses the Copilot review on #323. The bullets were PR-body prose in a changelog: CONTRIBUTING.md asks for a few sentences at most per entry, and the release workflow lifts the whole version section verbatim into the GitHub Release body, where an over-long section has actually broken a release here before. Kept one bullet per rollout facet so the change is still reconstructable from the changelog alone, but cut each to the observable change. The [Unreleased] section drops from 6009 to 4439 characters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Benchmarks14 regressions Highlights
Collections (488)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( |
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.
Implements #311 — the span-keyed lookup lane — end to end, including the full parity rollout.
The problem
grep -rn "ReadOnlySpan<char>" src/Celerity/Collectionsreturned zero hits before this PR. A tokenizer, CSV/log reader, or route dispatcher holding a slice of its input buffer had to callnew string(span)to probe any Celerity string-keyed type — one allocation plus a copy per lookup, on the hot path of exactly the workloads 23 string hashers, a perfect-hashFrozenCelerityDictionary, and aTrieexist to serve. .NET 9 closed that gap for the BCLDictionarywithGetAlternateLookup<ReadOnlySpan<char>>;net8.0is this library's floor, so it had no answer at all.What changed
1.
ISpanHashProvider(Celerity.Hashing)A sibling interface, not an overload on
IHashProvider<T>: that interface is generic in its key type, and aref structcould not be a generic type argument beforeallows ref struct(C# 13 / .NET 9). Expressing the span overload as a non-generic sibling sidesteps that entirely — the span is a method parameter, not a type argument. Same shape as the existingIHashProvider64<T>sibling.All 23 built-in
String*Hashertypes implement it. Each now has exactly one body:Hash(string)null-checks and delegates to the span overload (or, for the 64-bit hashers, both public entry points route through a shared privateHash64Core(ReadOnlySpan<char>)), so the two overloads cannot drift apart.2. Span lookups on the string-keyed collections
FrozenCelerityDictionary<TValue, THasher>TryGetValue(ReadOnlySpan<char>, out TValue?),ContainsKey(ReadOnlySpan<char>)FrozenCeleritySet<THasher>Contains(ReadOnlySpan<char>)CelerityDictionary<string, TValue, THasher>TryGetValue(ReadOnlySpan<char>, out TValue?),ContainsKey(ReadOnlySpan<char>)CeleritySet<string, THasher>Contains(ReadOnlySpan<char>)Trie<TValue>TryGetValue(...),ContainsKey(...),ContainsPrefix(...)On the four hashed types these are extension methods (
SpanLookupExtensions). AddingISpanHashProviderto a shipped class'sTHasherconstraint would break every existing instantiation, so the extra constraint lives on the methods instead: they bind only when the hasher supplies both interfaces, resolve statically (no boxing — the JIT still devirtualizes through the struct type parameter), and read like instance methods at the call site. The probes themselves are internal generic methods on each collection, mirroring the existing string probe exactly (including the frozen types' perfect and linear-probing-fallback shapes with the sameMix/seed).Trietakes no hasher, so its overloads are ordinary instance methods and itsFindNodewas generalized to a span so both surfaces share one descent.An empty span means
""(an ordinary key), never the out-of-bandnullkey — a span has no null state. Pinned by tests.3.
StringInternTable/StringInternTable<THasher>GetOrAdd(ReadOnlySpan<char>)returns the one canonicalstringfor those characters and allocates only on a miss. A 10M-cell parse over 100 distinct tokens creates 100 strings, not 10,000,000.This is the one collection you cannot build on the pre-.NET-9 BCL:
HashSet<string>.TryGetValuetakes astring, so you must allocate the string before you can discover you already had it.string.Internis not a substitute — process-wide, never collected, and still needs astring.Defaults to
StringFnV1AFullHasherrather than the frozen family's low-byteStringFnV1AHasher, because an intern table's inputs are arbitrary parsed text rather than curated identifiers; documented, and a collision is never a correctness problem (the ordinal span compare resolves it).Parity rollout
StringInternTableTests.cs,StringInternTableDifferentialTests.cs(seeded differential vs aDictionary<string,string>oracle, both a weak and a strong hasher)SpanLookupTests.cs— one block per type, each asserting the span result against that type's own string overload rather than a hard-coded expectation.SpanHashParityTests.cs— roster test + a per-hasherHash(s) == Hash(s.AsSpan())assertion over every length class, checked as a whole string, as a slice of a padded string, and as a slice of a caller-ownedchar[]StringInternTableBenchmark.cs([MemoryDiagnoser],[Params(1000, 100_000)] ItemCount,Dedupe+Lookupcategories,HashSet<string>baseline) registered inProgram.cs'sCoreBenchmarks; newSpanLookupcategory onFrozenCelerityDictionaryBenchmarkandTrieBenchmarkweb/index.htmlship card;COLLECTIONSentries inweb/dev/bench/index.htmlanddetail.html, plusSpanLookupadded to the Trie and FrozenCelerityDictionaryopsdocs/api/hashing.md→ISpanHashProvidersection;docs/api/collections.md→ Span-keyed lookups +StringInternTablesections,Triemethod table rows, and pointers from the four hashed types' sections;README.md→ collections list, "Choosing a collection" (two new rows), "Choosing a hasher" note, "API at a glance"[Unreleased] → Added2.4.0span-keyed-lookups entry flipped todonewith the design rationaleCelerity.FuzzStringInternTabletarget;Celerity.AotSmokeTestblock exercising every new span instantiation, the hasher parity contract, and the intern table (incl. a second closed generic so the ILC generates more than one)Deliberately out of scope, per the issue's own scoping paragraph: the optional
ReadOnlySpan<byte>UTF-8 axis and the#if NET9_0_OR_GREATERIAlternateEqualityComparerplumbing. Neither is needed for the workload win, and each would widen a brand-new public abstraction before it is load-bearing. Noted inROADMAP.mdso it is a recorded decision, not drift.Test plan
dotnet buildclean across the fullnet8.0;net9.0;net10.0matrix — no new warnings (GenerateDocumentationFileis on and CS1591 is an error, so every new public member is documented)dotnet test— 5009 passed, 0 failed onnet8.0and onnet10.0; 116 of those are newcoverage.runsettings+scripts/coverage_report.py: identical to the pre-change baseline on the same machine (99.98% line / 99.82% branch vs 99.98% / 99.81%). The residual local shortfall is pre-existing and lives entirely inBitSet,Hash64Source,SimdReductionsand the frozen types' unreachable 2^30 ceilingbreak— no new uncovered line or branch is introduced by this PR. CI is the authority on the 100% gateCelerity.Fuzz --target StringInternTable --iterations 2000— all cases passedCelerity.AotSmokeTestrun (managed,net10.0) — all checks passed; the Native AOT publish job in CI is the real gate for the newSpanLookupExtensionsmethod-generic instantiationsmain— the new StringInternTable card and the SpanLookup rows on the Trie / FrozenCelerityDictionary cards only render oncedata.jsis republished frommainCloses #311
🤖 Generated with Claude Code