feat(collections): add BTreeDictionary and BTreeSet — the library's first sorted map and set - #320
Conversation
Coverage
Files below 100% line coverage
|
There was a problem hiding this comment.
Pull request overview
Adds Celerity’s first sorted map/set by introducing B-tree–backed ordered collections (fan-out 32 via MinDegree = 16) with allocation-free enumeration and an ordered API surface (min/max, bounds, and O(log n + k) range scans). The PR also wires the new collections through parity tests, differential fuzzing, benchmarks, docs, and the web benchmark dashboard, aligning with the roadmap item for ordered containers (Closes #305).
Changes:
- Add
BTreeDictionary<TKey, TValue, TComparer>andBTreeSet<T, TComparer>plusDefaultComparer<T>(struct comparer) to enable devirtualized comparisons. - Add extensive unit + differential tests, fuzz targets, AOT smoke coverage, and BenchmarkDotNet benchmarks; wire benchmarks into the web dashboard.
- Update documentation (README + API docs), ROADMAP, and CHANGELOG for the new ordered containers.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Celerity/Collections/BTreeDictionary.cs | New B-tree sorted dictionary implementation + ordered APIs + IDictionary/IReadOnlyDictionary support. |
| src/Celerity/Collections/BTreeSet.cs | New B-tree sorted set implementation + ordered APIs + ISet/IReadOnlySet support. |
| src/Celerity/Collections/DefaultComparer.cs | Adds default struct comparer wrapper over Comparer<T>.Default for zero-cost ordering. |
| src/Celerity.Tests/Collections/BTreeDictionaryTests.cs | Dedicated behavioral tests for BTreeDictionary. |
| src/Celerity.Tests/Collections/BTreeDictionaryEnumerationTests.cs | Enumeration/versioning/range-enumeration tests for BTreeDictionary. |
| src/Celerity.Tests/Collections/BTreeDictionaryDifferentialTests.cs | CsCheck differential tests vs SortedDictionary (order-sensitive). |
| src/Celerity.Tests/Collections/BTreeSetTests.cs | Dedicated behavioral tests for BTreeSet. |
| src/Celerity.Tests/Collections/BTreeSetEnumerationTests.cs | Enumeration/versioning/range-enumeration tests for BTreeSet. |
| src/Celerity.Tests/Collections/BTreeSetDifferentialTests.cs | CsCheck differential tests vs SortedSet (order-sensitive). |
| src/Celerity.Tests/Collections/AddAndTryAddTests.cs | Adds shared-suite coverage rows for BTreeDictionary add/try-add semantics. |
| src/Celerity.Tests/Collections/ContainsValueTests.cs | Adds shared-suite coverage for BTreeDictionary ContainsValue. |
| src/Celerity.Tests/Collections/IEnumerableConstructorTests.cs | Adds shared-suite coverage for BTreeDictionary enumerable constructor. |
| src/Celerity.Tests/Collections/IndexerOverwriteEnumerationTests.cs | Adds parity coverage that indexer overwrite doesn’t invalidate enumeration (BTreeDictionary). |
| src/Celerity.Tests/Collections/IndexerReturnTypeTests.cs | Pins BTreeDictionary primary indexer return type to non-nullable TValue. |
| src/Celerity.Tests/Collections/ReadOnlyDictionaryInterfaceTests.cs | Validates IReadOnlyDictionary behavior and ordered Keys/Values for BTreeDictionary. |
| src/Celerity.Tests/Collections/RemoveOutValueTests.cs | Adds shared-suite coverage for Remove(key, out value) on BTreeDictionary. |
| src/Celerity.Tests/Collections/SetIEnumerableConstructorTests.cs | Adds shared-suite coverage for BTreeSet enumerable constructor invariants. |
| src/Celerity.Tests/Collections/TryAddDuplicateResizeTests.cs | Adds enumerator-validity tests for duplicate vs split-triggering inserts at node capacity. |
| src/Celerity.Fuzz/Differential.cs | Registers B-tree fuzz targets and adds order-sensitive oracle comparisons. |
| src/Celerity.Benchmarks/BTreeDictionaryBenchmark.cs | Adds BTreeDictionary benchmarks incl. mixed workload + range scan. |
| src/Celerity.Benchmarks/BTreeSetBenchmark.cs | Adds BTreeSet benchmarks incl. mixed workload + range scan. |
| src/Celerity.Benchmarks/Program.cs | Registers new B-tree benchmarks in the core suite. |
| src/Celerity.AotSmokeTest/Program.cs | Adds Native AOT smoke coverage for both B-tree types + inline-array enumerators + custom struct comparer instantiation. |
| web/index.html | Adds “What ships in the box” cards for BTreeDictionary/BTreeSet. |
| web/dev/bench/index.html | Adds benchmark dashboard wiring for B-tree collections + baseline type parsing. |
| web/dev/bench/detail.html | Adds benchmark detail page wiring for B-tree collections + baseline type parsing. |
| docs/api/collections.md | Adds full API documentation sections for BTreeDictionary and BTreeSet. |
| README.md | Documents new ordered collections and updates “Choosing a collection” guidance. |
| ROADMAP.md | Marks ordered-container roadmap item as done and links issue #305. |
| CHANGELOG.md | Adds [Unreleased] entries for the new ordered collections and comparer helper. |
- README: the mutable hash-based sets implement ISet<T>, not IReadOnlySet<T> (that gap is what #306 tracks), so the "not the right answer when" paragraph no longer claims otherwise. - CHANGELOG: fold the three [Unreleased] bullets into one tight entry ending in Closes #305, per the terse-entry convention in CLAUDE.md / CONTRIBUTING.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
docs/api/collections.md:3862
- This sentence says a
defaultelement “sorts first”, but ordering is defined byTComparer. For value-typeT,default(T)does not necessarily sort first (e.g., -1 precedes 0 forint). Consider documentingnullordering (when applicable) separately and treatingdefault(T)as just another legal element.
Membership is defined by `TComparer` — two elements are the same element when the comparer orders them equal. The set-algebra members materialize the right-hand side into a `HashSet<T>`, so they compare *that* side with `EqualityComparer<T>.Default` (matching the rest of the family). A custom comparer that treats two values as equal when `EqualityComparer<T>.Default` does not — a case-insensitive order, say — can therefore disagree with `SortedSet<T>` on those members alone. A `null` (or `default`) element is legal and sorts first. Not thread-safe.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/Celerity/Collections/BTreeDictionary.cs:47
- In the
TComparertype-parameter docs, the crefDefaultComparer{T}refers to an out-of-scopeT. This comparer is for keys, so the cref should beDefaultComparer{TKey}.
/// The comparer that defines the key order. Must be a value type implementing <see cref="IComparer{TKey}"/>
/// so the JIT can devirtualize and inline it — an interface-typed comparer would cost a virtual call for
/// every key inspected inside a node. Use <see cref="DefaultComparer{T}"/> (or the two-parameter
/// <see cref="BTreeDictionary{TKey, TValue}"/> alias) for the natural order.
src/Celerity/Collections/BTreeDictionary.cs:80
- The remarks still use
Comparer{T}.Default(whereTis not in scope for this type). This should reference the key type (Comparer{TKey}.Default) to avoid invalid/broken cref links.
/// Unlike <see cref="SortedDictionary{TKey, TValue}"/>, a <c>null</c> key is legal:
/// <see cref="Comparer{T}.Default"/> orders <c>null</c> before every non-<c>null</c> key (a custom
/// <typeparamref name="TComparer"/> that rejects <c>null</c> overrides that). There is no out-of-band
src/Celerity/Collections/BTreeDictionary.cs:304
ContainsValueXML docs referenceEqualityComparer{T}.Default, butTis not a type parameter here; the comparison is against the value type. UsingEqualityComparer{TValue}.Defaultkeeps the cref valid and matches the implementation intent.
/// <summary>
/// Determines whether any entry holds <paramref name="value"/>, comparing with
/// <see cref="EqualityComparer{T}.Default"/>. This is an <c>O(n)</c> scan — the tree is indexed by key,
/// not by value.
…irst sorted map and set Celerity shipped 38 collections and not one sorted map or set: `Trie` was the only ordered type and it is string-keyed. This adds a cache-friendly B-tree map and set with fan-out 32 (up to 31 keys per node in flat arrays), so a lookup visits log32(n) nodes instead of chasing the log2(n) pointers a red-black tree costs — roughly 4 cache misses instead of ~20 at n = 1M. Ordering is a `struct, IComparer<T>` type parameter, mirroring the struct hashers, so the comparison inlines instead of costing a virtual call for every key inspected inside a node; `DefaultComparer<T>` and the one/two-parameter aliases close over `Comparer<T>.Default` for the common case. Beyond the map/set core the types add the ordered surface a hash table cannot answer: Min, Max, TryGetLowerBound, TryGetUpperBound, EnumerateRange in O(log n + k), and in-order enumeration. Both enumerators hold their traversal path in an `[InlineArray]` buffer, so a foreach allocates nothing. Insertion splits bottom-up rather than preemptively on the way down, so a rejected duplicate `TryAdd` never restructures the tree or invalidates an active enumerator; an in-place indexer overwrite does not bump the version either, matching BCL Dictionary and the rest of the family (#233). Parity rollout in the same change: - Dedicated tests: BTreeDictionaryTests, BTreeSetTests, and the two *EnumerationTests, covering splits at 1/31/32/512/4000 entries, the three whole-tree deletion orders that drive borrow-left, borrow-right and merge, the internal-node predecessor/successor swap, null/default keys, the bounds at every probe of a 600-key tree, and enumerator invalidation. - Differential tests: BTreeDictionaryDifferentialTests / BTreeSetDifferentialTests — CsCheck against SortedDictionary/SortedSet, asserting the enumerated *sequence*, not just membership, plus the ordered surface and set algebra. - Cross-collection suites: rows added to AddAndTryAddTests, ContainsValueTests, RemoveOutValueTests, TryAddDuplicateResizeTests, IEnumerableConstructorTests, SetIEnumerableConstructorTests, ReadOnlyDictionaryInterfaceTests, IndexerReturnTypeTests and IndexerOverwriteEnumerationTests. - Fuzz: BTreeDictionary / BTreeSet targets in Differential.All, order-sensitive against the sorted BCL oracles, with a key domain wide enough to split nodes. - Benchmarks: BTreeDictionaryBenchmark / BTreeSetBenchmark registered in the CI core suite, with the mixed insert + lookup + range-scan row. - Dashboard: COLLECTIONS entries in web/dev/bench/{index,detail}.html (plus SortedDictionary/SortedSet in BCL_TYPES so the baseline parses) and ship cards in web/index.html. - Docs: docs/api/collections.md sections, README collection list, decision-table row, quick-start block, and the closing guidance that used to send readers to the BCL for ordered iteration. - CHANGELOG under [Unreleased]; ROADMAP entry flipped to done. Closes #305. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…age gaps The local short-run sweep at 100k entries puts BTreeDictionary at 0.59x SortedDictionary on the mixed insert + lookup + range-scan workload and 0.36x its allocation, but 1.12x on a delete-dominated load and no better at n = 1000. Say so in the XML remarks and the API reference rather than only claiming the win, per Guiding Principle #5. Also covers the non-generic IEnumerator.Current on the key/value/range enumerators, which were the only lines the suite missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- README: the mutable hash-based sets implement ISet<T>, not IReadOnlySet<T> (that gap is what #306 tracks), so the "not the right answer when" paragraph no longer claims otherwise. - CHANGELOG: fold the three [Unreleased] bullets into one tight entry ending in Closes #305, per the terse-entry convention in CLAUDE.md / CONTRIBUTING.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comparer<T>.Default orders null before every non-null value, which is what makes a null key or element legal here — but that says nothing about default(TKey) for a value type: default(int) is 0, and sorts after every negative key like any other. The B-trees have no out-of-band default slot at all, unlike the hash-based family, so default(TKey) is simply an ordinary key. Reworded in the BTreeDictionary / BTreeSet / DefaultComparer XML remarks and in both places in docs/api/collections.md, and renamed the two tests whose names claimed default(TKey) sorts first (the assertions were already correct: 0 lands between -5 and 5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A leaf holds a key and a value array; only an internal node adds a child array, so "three arrays per 31 entries" overstated a fixed count. The XML remark was already corrected; this brings docs/api/collections.md in line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…v2.4.0 cut v2.4.0 was tagged on main while this branch was open, which inserted a "## [2.4.0]" heading directly under "## [Unreleased]". The rebase carried this PR's entry into the released section; it belongs above it, under a fresh [Unreleased] -> Added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7cfa6ac to
f80e5d6
Compare
|
Rebased onto One thing worth flagging, because the rebase did not conflict and so would have gone unnoticed: the release commit inserted a Re-verified after the rebase: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
CHANGELOG.md:9
- The new
[Unreleased]changelog entry is still longer/more implementation-detailed than the repo’s changelog guidance (“a few sentences at most”, user-facing). Consider tightening it to 1–2 short sentences focusing on the shipped surface and why it matters, and leave the deeper perf/implementation discussion to the PR body/docs.
- **`BTreeDictionary<TKey, TValue, TComparer>` and `BTreeSet<T, TComparer>`** (with `BTreeDictionary<TKey, TValue>` / `BTreeSet<T>` aliases and the `DefaultComparer<T>` struct comparer) in `Celerity.Collections` — the library's first sorted map and set, and the B-tree the BCL lacks. Up to 31 keys per node keep a lookup `log₃₂(n)` node visits deep instead of chasing the `log₂(n)` pointers a red-black tree costs, and both add the ordered surface a hash table cannot answer: `Min` / `Max`, lower / upper bound, `EnumerateRange` in `O(log n + k)`, and in-order enumeration. They win on the interleaved insert + lookup + range-scan workload and on memory, and lose slightly on a delete-dominated one. Not thread-safe. Closes [#305](https://github.com/marius-bughiu/Celerity/issues/305).
… index is past the end CopyTo on the dictionary and on the key/value views validated a negative index but let an index beyond the destination's length fall through to the insufficient-space ArgumentException. SetOperations.CopyTo — which BTreeSet and the rest of the set family route through — and BCL Dictionary.CopyTo both report that as ArgumentOutOfRangeException, so the three overloads now do too. Covered by the existing CopyTo validation tests, extended with the past-the-end index for each of the three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/Celerity.Benchmarks/BTreeDictionaryBenchmark.cs:250
- BTreeDictionary_Mixed is intended to include an in-order range scan, but this block currently enumerates a fixed prefix from the start of the map (and ignores rangeFrom/rangeTo). Using EnumerateRange here would better match the documented workload and keep the mixed row aligned with the RangeScan benchmark.
if ((i & 1023) == 0)
{
int scanned = 0;
foreach (KeyValuePair<int, int> entry in map)
{
src/Celerity.Benchmarks/BTreeSetBenchmark.cs:230
- BTreeSet_Mixed is intended to include an in-order range scan, but this block currently enumerates a fixed prefix from the start of the set and ignores rangeFrom/rangeTo. Using EnumerateRange here would better match the documented workload and keep the mixed row aligned with RangeScan.
if ((i & 1023) == 0)
{
int scanned = 0;
foreach (int item in set)
{
The Mixed groups interleave inserts and lookups with an in-order scan of the leading window, not a windowed EnumerateRange — the comments claimed the latter. The scan is deliberately the same walk on both sides so the row measures insert + lookup + enumeration on symmetric work; the range-API asymmetry is isolated in the RangeScan group, which would otherwise dominate it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Benchmarks65 regressions Highlights
Collections (460)
Hashers (100)
Same-runner A/B (sharded 8-way): main ( |
"and it string-keyed" reads as a missing word; the ordered-lane sentence now ends "and that one string-keyed". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
CHANGELOG.md:9
- The new
[Unreleased]entry is significantly longer than the repo’s changelog guidance (“a few sentences at most”) and includes implementation-detail phrasing. Consider tightening it to a short, user-facing summary so future releases don’t risk oversized release bodies.
- **`BTreeDictionary<TKey, TValue, TComparer>` and `BTreeSet<T, TComparer>`** (with `BTreeDictionary<TKey, TValue>` / `BTreeSet<T>` aliases and the `DefaultComparer<T>` struct comparer) in `Celerity.Collections` — the library's first sorted map and set, and the B-tree the BCL lacks. Up to 31 keys per node keep a lookup `log₃₂(n)` node visits deep instead of chasing the `log₂(n)` pointers a red-black tree costs, and both add the ordered surface a hash table cannot answer: `Min` / `Max`, lower / upper bound, `EnumerateRange` in `O(log n + k)`, and in-order enumeration. They win on the interleaved insert + lookup + range-scan workload and on memory, and lose slightly on a delete-dominated one. Not thread-safe. Closes [#305](https://github.com/marius-bughiu/Celerity/issues/305).
|
Review loop is quiet — the latest Copilot pass (on Three threads are deliberately left unresolved for your call, each with the reasoning in-thread:
One more, from the suppressed low-confidence block rather than a thread: Copilot has twice asked for a shorter |
Closes #305.
Celerity shipped 38 collections and not one sorted map or set —
Triewas the only ordered type and it is string-keyed. This adds the pair the roadmap's ordered lane opens with: a cache-friendly B-tree map and set with fan-out 32 (up to 31 keys per node in flat arrays), so a lookup visitslog₃₂(n)nodes instead of chasing thelog₂(n)pointers a red-black tree costs — roughly 4 cache misses instead of ~20 atn = 1M.Design
where TComparer : struct, IComparer<TKey>, mirroring the struct hashers, so the comparison inlines instead of costing a virtual call for every key inspected inside a node.DefaultComparer<T>wrapsComparer<T>.Default, and theBTreeDictionary<TKey, TValue>/BTreeSet<T>aliases close over it exactly asIntDictionary<TValue>frontsIntDictionary<TValue, THasher>. There are also(TComparer)constructor overloads so a stateful comparer is not assumed to be default-constructed.MinDegree = 16→ 31 keys, 124–248 bytes for 4- and 8-byte keys = 2–4 cache lines), as the issue asked.Min,Max,TryGetMin/TryGetMax,TryGetLowerBound,TryGetUpperBound,EnumerateRange(from, toExclusive)inO(log n + k), and in-order enumeration. The bounds use theTry…naming the BCL uses for may-not-exist results rather than the issue's bareLowerBound(key).BTreeDictionaryimplementsIDictionary<TKey, TValue?>andIReadOnlyDictionary<TKey, TValue?>;BTreeSetimplementsISet<T>andIReadOnlySet<T>— so the new type does not re-open the Implement IReadOnlySet<T> on the ten mutable sets #306 / Implement IDictionary<TKey,TValue> on the dictionary family — the mirror of the set-interface gap #307 gaps.[InlineArray]buffer (the only inline arrays in the library, hence the new AOT-smoke coverage), so aforeachallocates nothing.TryAddnever restructures the tree or invalidates an active enumerator. An in-place indexer overwrite does not bump the version either, matching BCLDictionaryand the family convention pinned byIndexerOverwriteEnumerationTests(Indexer overwrite of an existing key spuriously invalidates enumerators (BCL parity) #233); the primary indexer returns the non-nullableTValuethatIndexerReturnTypeTestspins (CelerityDictionary indexer get returns TValue? while IntDictionary/LongDictionary return TValue #88).null/defaultkey or element is legal and sorts first, matching how the family treats the out-of-band default key.Measured — the issue's kill criterion
Local short-run sweep (
--filter '*BTree*' --job short), ratios vs the BCL baseline at 100k items:BTreeDictionaryvsSortedDictionaryBTreeSetvsSortedSet¹
SortedDictionaryhas no range view, so the baseline is the best a caller can do: enumerate from the start and break at the upper bound.² Against
SortedSet.GetViewBetween— the BCL's own range view, so this one is a like-for-like comparison.The mixed insert + lookup + range-scan row — the documented win workload the issue names — is a 1.7x win for the dictionary and 1.27x for the set, so the kill criterion is not met. The two honest losses (a delete-dominated load a few percent behind, and no win at
n = 1000) are now documented in the XML remarks and the API reference rather than glossed over.Parity rollout (all in this PR)
BTreeDictionaryTests,BTreeSetTests,BTreeDictionaryEnumerationTests,BTreeSetEnumerationTests: splits at 1/31/32/512/4000 entries, ascending/descending/shuffled inserts, three whole-tree deletion orders that drive borrow-left, borrow-right and merge, the internal-node predecessor/successor swap,default/nullkeys, both bounds at every probe of a 600-key tree, a 20k-operation interleaved reconciliation, and enumerator invalidation (including the cases that must not invalidate).AddAndTryAddTests,ContainsValueTests,RemoveOutValueTests,TryAddDuplicateResizeTests,IEnumerableConstructorTests,SetIEnumerableConstructorTests,ReadOnlyDictionaryInterfaceTests,IndexerReturnTypeTests,IndexerOverwriteEnumerationTests. Genuinely N/A and noted in-file:ConstructorValidationTests/SetConstructorValidationTests/LoadFactorBoundaryTests/IEnumerableConstructorNullPriorityTests/EnsureCapacityAndTrimExcessTests(no capacity or load-factor surface — a B-tree grows a node at a time),TryAddProbeCountTests/BulkConstructorNoResizeTests(no hasher, no probe chain).BTreeDictionaryDifferentialTests/BTreeSetDifferentialTests(CsCheck againstSortedDictionary/SortedSet, asserting the enumerated sequence and the ordered surface and set algebra, not just membership), plusBTreeDictionary/BTreeSettargets inDifferential.Allwith a key domain wide enough to actually split nodes. Both fuzz targets pass at 3–5k cases locally.BTreeDictionaryBenchmark/BTreeSetBenchmark,ItemCountparams, per-Remove[IterationSetup], BCL methodBaseline = true, and the mixed row; registered inProgram.cs's core suite.COLLECTIONSentries inweb/dev/bench/index.htmlanddetail.html,SortedDictionary/SortedSetadded toBCL_TYPESso the baseline actually parses, and ship cards inweb/index.html.docs/api/collections.mdsections following the per-type template; README collection list, decision-table row, quick-start block, and the closing guidance that used to send readers to the BCL for ordered iteration.Celerity.AotSmokeTestblock exercising both types, a hand-written struct comparer (a second closed generic for ILC), the split/rebalance paths, and both enumerators.[Unreleased]; (g) ROADMAP entry flipped todoneand the "not one sorted map or set" framing updated.Test plan
dotnet buildclean onnet8.0/net9.0/net10.0(no new warnings; CS1591 gate green).dotnet test— 4,595 tests pass on all three TFMs.Celerity.Fuzz—--target BTreeDictionary3,000 cases and--target BTreeSet5,000 cases pass; full--target allsweep still passes.main— the newBTreeDictionary/BTreeSetcards should populate from the first post-merge benchmark run.🤖 Generated with Claude Code