feat(primitives): add SortedSpan — merge-based set algebra over sorted spans - #342
Merged
Conversation
…d spans Intersect / Union / Except write straight into a caller-owned Span<T>; IntersectCount and Overlaps answer with no buffer at all. The BCL has no set operation over spans anywhere, so the alternatives (HashSet<T>.IntersectWith, LINQ Intersect) allocate a table and hash every element rather than using the order the data already has. A two-cursor linear merge touches each element once. On two 1M-element sorted int spans over a 2M universe: intersect 6.1 ms vs 25.7 ms for HashSet<int> (4.2x; 5.7x vs LINQ), union 4.0x, except 2.8x, and 0 bytes allocated against 17.9 MB. When one side is at least 32x the other the merge switches to exponential (galloping) search of the long side, which is where the win gets large: 1k against 10M intersects in 0.37 ms vs 94.3 ms — 257x, and 422x for IntersectCount. That clears the issue's kill criterion with room to spare. Union deliberately has no galloping path (its result is at least as long as the longer input, so writing the answer out dominates) and Except gallops only when the subtrahend is the long side, for the same reason. The Vector256 path was not shipped, per the issue's own condition: the scalar merge is already memory-bound at 1M x 1M and merge is branch-heavy enough that vectorizing it is frequently a wash. Inputs must be sorted ascending — unsorted input silently returns a wrong answer. The precondition leads every doc surface and is asserted in Debug builds only; a Release check would cost exactly what the algorithm saves. Duplicates within an input are collapsed, so every result is strictly ascending and matches what HashSet<T> computes for the same values. Full rollout in one change: dedicated + CsCheck differential tests (100% line and branch on the new type), a Celerity.Fuzz target, Native AOT smoke coverage, SortedSpanBenchmark registered in the CI suite with the matching dashboard card on both bench pages, a landing-page ship card, utilities API reference and README sections including a "choosing a collection" row, and the ROADMAP status flipped to done. Closes #313. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds SortedSpan to Celerity.Primitives: allocation-free, merge-based set algebra over already ascending-sorted spans (with galloping for highly asymmetric sizes), plus tests, benchmarks, fuzz/AOT coverage, and documentation/dashboard wiring.
Changes:
- Introduces
SortedSpanAPIs (Intersect/Union/Except/IntersectCount/Overlaps) with duplicate-collapsing set semantics and Debug-only sortedness assertions. - Adds comprehensive unit + differential tests, fuzz target, benchmarks, and AOT smoke coverage for the new surface.
- Updates docs/README/changelog/roadmap and benchmark dashboard pages to surface the new utility.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/index.html | Adds “SortedSpan” ship card to the public landing page. |
| web/dev/bench/index.html | Registers a SortedSpan benchmark card + adds Linq as a baseline type. |
| web/dev/bench/detail.html | Adds SortedSpan to the per-card detail page + adds Linq as a baseline type. |
| src/Celerity.Tests/Utils/SortedSpanTests.cs | Adds dedicated correctness tests for merge/gallop paths and edge cases. |
| src/Celerity.Tests/Utils/SortedSpanDifferentialTests.cs | Adds CsCheck property-based differential tests vs HashSet<int> oracle. |
| src/Celerity.Tests/Packaging/PackageSplitTests.cs | Verifies SortedSpan is in the Celerity.Primitives assembly. |
| src/Celerity.Primitives/SortedSpan.cs | Implements the new sorted-span set algebra APIs and helpers. |
| src/Celerity.Primitives/README.md | Documents SortedSpan in the primitives package readme. |
| src/Celerity.Primitives/Celerity.Primitives.csproj | Updates package description to mention sorted-span set algebra. |
| src/Celerity.Fuzz/Differential.cs | Adds SortedSpan differential fuzz target. |
| src/Celerity.Benchmarks/SortedSpanBenchmark.cs | Adds BenchmarkDotNet coverage vs HashSet and LINQ baselines. |
| src/Celerity.Benchmarks/Program.cs | Registers SortedSpanBenchmark in the benchmark suite. |
| src/Celerity.AotSmokeTest/Program.cs | Exercises SortedSpan APIs under (Native) AOT smoke coverage. |
| ROADMAP.md | Marks the roadmap item as done and records design decisions/measurements. |
| README.md | Adds SortedSpan to package table + “choosing a collection” guidance + example. |
| docs/api/utilities.md | Adds an API reference section and usage examples for SortedSpan. |
| CHANGELOG.md | Adds Unreleased entries describing SortedSpan + its test/bench/docs wiring. |
The ArgumentException carried a hardcoded "destination" string literal, which would silently drift if the parameter were renamed. The helper now takes the name and Append passes nameof(destination). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/Celerity.Primitives/SortedSpan.cs:241
- In MergeIntersect, the y < x branch increments j by 1, which can repeatedly compare the same unmatched duplicate values. Since duplicates are treated as a single set element, you can skip over all equal y values when y < x (as other merge paths already do) to reduce work for duplicate-heavy inputs.
else if (y < x)
{
if (++j == b.Length)
break;
}
src/Celerity.Primitives/SortedSpan.cs:351
- In MergeCount, the y < x branch increments j by 1. Since repeated y values are treated as a single set element, you can skip over duplicates when y < x (as in other merge routines) to reduce the number of comparisons for duplicate-heavy inputs.
else if (y < x)
{
if (++j == b.Length)
break;
}
src/Celerity.Primitives/SortedSpan.cs:236
- In MergeIntersect, the x < y branch advances i by 1 even though duplicates are defined to be collapsed. When x < y, any subsequent duplicates of x are also < y, so re-checking them is unnecessary work in a hot loop (and inconsistent with MergeUnion/MergeExcept which use SkipEqual on mismatches). Skipping duplicates here reduces comparisons for duplicate-heavy inputs without changing semantics.
This issue also appears on line 237 of the same file.
if (x < y)
{
if (++i == a.Length)
break;
}
src/Celerity.Primitives/SortedSpan.cs:346
- In MergeCount, the x < y branch advances i by 1 rather than skipping duplicates. Because inputs may contain repeated values but semantics treat them as one element, duplicates that are still < y can be skipped safely; this avoids redundant comparisons in the count/overlaps hot path.
This issue also appears on line 347 of the same file.
if (x < y)
{
if (++i == a.Length)
break;
}
… whole run The mismatch branches of the intersect and count merges look like they should skip the equal run the way union and except do. They should not: the run-skip costs an equality test per element that can only pay off on duplicates, and this branch is the entire loop on distinct inputs. Measured a wash on duplicate-heavy data and 1.66x slower on interleaved-disjoint 1M x 1M spans. Comment only — no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
Re: the four low-confidence findings on the merge mismatch branchesThe second review round raised no inline comments but its body carries four suppressed findings, all making the same claim: It is a performance claim, so I measured it rather than argued it. Both variants, interleaved round by round with the leading variant alternated and medians taken over 41 rounds, so warm-up and cache ordering cancel (a naive back-to-back run reverses its own verdict when you swap the order — which is how this nearly went the other way):
So: a wash on the two shapes the suggestion targets, and a 66% regression on the one where the mismatch branch matters most. The reason the premise does not hold is that the two conditions pull apart. On duplicate-heavy input both sides contain nearly every value, so almost every step takes the equality branch — which already skips runs — and the mismatch branch barely executes; there is nothing there to save. The shape that lives entirely in the mismatch branch is the distinct/disjoint one, and there The apparent inconsistency with Not changing the code. I have added the measurement and the reasoning as a comment on the merge section in 8416ada, so the next reader — human or reviewer — does not have to rediscover it. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Celerity.Primitives/SortedSpan.cs:579
- If
AssertSortedis tightened to catch unordered comparisons (e.g., NaN), the assertion message "is smaller" becomes inaccurate. Consider wording that covers both "smaller" and "unordered" cases.
$"SortedSpan requires '{paramName}' to be sorted in ascending order; element {unsortedAt} is smaller than element {unsortedAt - 1}.");
src/Celerity.Primitives/SortedSpan.cs:570
AssertSortedonly checksspan[i] < span[i - 1], which does not detect unordered comparisons (e.g.,float.NaNmakes both<and>false). That means Debug builds can fail to assert the documented sortedness precondition for spans containing NaN/unordered values.
This issue also appears on line 579 of the same file.
if (span[i] < span[i - 1])
The check tested span[i] < span[i - 1], which is false for a NaN pair in both
directions — so a span containing NaN sailed past an assertion whose whole job
is to catch exactly that, and which the XML docs already name as the way a
floating-point span violates the precondition. Testing !(span[i - 1] <= span[i])
catches a descending pair and an unordered one alike, and the message now says
"is not ordered after" rather than "is smaller".
Debug-only: the whole method is [Conditional("DEBUG")], so Release builds elide
the call and its scan as before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Both suppressed findings from the third round were right, and they are fixed in 882e20b.
Still Debug-only — the method is Re-verified locally after the change: 5289 tests pass and |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
CHANGELOG.md:12
- The new SortedSpan CHANGELOG entries are very long (multiple sentences with detailed benchmark numbers) and repeat supporting-artifact details across several bullets. This diverges from the repo’s changelog guidance (CLAUDE.md:43-49) to keep entries short and user-facing, and it increases the risk that the extracted release notes exceed GitHub’s release-body limit.
- **`SortedSpan`** in `Celerity.Primitives` — set algebra over spans that are **already sorted ascending**: `Intersect` / `Union` / `Except` write straight into a caller-owned `Span<T>`, and `IntersectCount` / `Overlaps` answer without a buffer at all. The BCL has no set operation over spans, so the alternatives (`HashSet<T>.IntersectWith`, LINQ `Intersect`) allocate a table and hash every element instead of using the order the data already has: a two-cursor merge runs **4.2x faster on two 1M-element `int` spans (6.1 ms vs 25.7 ms) allocating 0 bytes against 17.9 MB**, and when one side is 32x the other it gallops — **1k against 10M takes 0.37 ms vs 94.3 ms, 257x**. Inputs **must** be sorted ascending; unsorted input silently returns a wrong answer, asserted in Debug builds and deliberately unchecked in Release. Closes [#313](https://github.com/marius-bughiu/Celerity/issues/313).
- `SortedSpanTests` and `SortedSpanDifferentialTests` — dedicated coverage of the merge, the galloping path, the duplicate-collapsing set semantics and the destination-too-short contract, plus a CsCheck property test and a `Celerity.Fuzz` target reconciling every operation against a `HashSet<T>` oracle across length ratios, and Native AOT smoke coverage. Closes [#313](https://github.com/marius-bughiu/Celerity/issues/313).
- `SortedSpanBenchmark` in the CI-tracked suite and the matching **SortedSpan** dashboard card, with `HashSet<int>` set algebra as the baseline, LINQ arms alongside it, and an asymmetric row for the galloping shape. Closes [#313](https://github.com/marius-bughiu/Celerity/issues/313).
- Utilities-reference and README sections for `SortedSpan`, including a "choosing a collection" row and the sortedness caveat stated in the row itself. Closes [#313](https://github.com/marius-bughiu/Celerity/issues/313).
src/Celerity.Primitives/SortedSpan.cs:583
AssertSortedalways constructs the interpolated failure message because it’s passed directly toDebug.Assert(...)as an argument, even when the assertion holds. That’s an avoidable string allocation on every call in Debug builds (and the unused message includeselement -1when ordered). Consider only constructing the message on the failing path.
Debug.Assert(
unorderedAt == 0,
$"SortedSpan requires '{paramName}' to be sorted in ascending order; element {unorderedAt} is not ordered after element {unorderedAt - 1} (it is smaller, or the two are unordered — e.g. NaN).");
The four bullets carried the full pitch, the mechanism and every supporting detail. CLAUDE.md asks for short, user-facing entries, and the release workflow extracts the whole version section verbatim as the GitHub Release body, which this repo has overrun before. Same facts, ~300 characters shorter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
Round four: one taken, one measured and declinedCHANGELOG length — fair, fixed in a8c3b43. The four bullets carried the full pitch plus every supporting detail, and
Verified rather than asserted — a The probe method inside the interpolation hole is called zero times and 100,000 passing asserts allocate zero bytes, because overload resolution picks |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Celerity.Primitives/SortedSpan.cs:114
destinationoverlap/aliasing isn’t documented, but the merge reads froma/bafter writing todestination, so overlapping spans can corrupt subsequent reads and produce wrong results. Please document thatdestinationmust not overlap the source spans (or, if overlap is intended, the algorithm would need to be made overlap-safe).
/// <param name="destination">
/// Receives the result. <c>a.Length + b.Length</c> elements is always enough.
/// </param>
src/Celerity.Primitives/SortedSpan.cs:144
- Like
Intersect/Union,Exceptwill misbehave ifdestinationoverlapsaorb(writes can overwrite elements that haven’t been read yet). The API docs currently don’t call this out, so callers may reasonably assume in-place use is supported. Please document the non-overlap requirement in the destination parameter docs.
/// <param name="a">The span to subtract from. Must be sorted ascending.</param>
/// <param name="b">The span of values to remove. Must be sorted ascending.</param>
/// <param name="destination">Receives the result. <c>a.Length</c> elements is always enough.</param>
/// <returns>The number of values written — the result is <c>destination[..returned]</c>.</returns>
src/Celerity.Primitives/SortedSpan.cs:81
- The implementation writes into
destinationwhile still reading froma/b, so ifdestinationoverlaps either input span the result can become incorrect (reads can observe overwritten values). The public API currently doesn’t document whether aliasing/overlap is allowed; this should be stated explicitly in the XML docs to prevent accidental in-place use.
This issue also appears in the following locations of the same file:
- line 112
- line 141
/// <param name="destination">
/// Receives the result. <c>min(a.Length, b.Length)</c> elements is always enough.
/// </param>
… an input The merge writes its result while it is still reading both sources, so a destination overlapping either input can overwrite elements that have not been consumed yet — silently, in the same way unsorted input does. The contract said nothing about it, so a caller could reasonably have assumed in-place use worked. The non-overlap requirement is now documented on the type, on every destination parameter and in the utilities reference, and enforced the same way the ordering precondition is: a Debug-only assert over MemoryExtensions.Overlaps, elided from Release along with the rest of the precondition checking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Round five was right on all three counts — same finding stated three times — and it is fixed in aa6477a.
The requirement is now stated in three places (the type remarks alongside the ordering precondition, every Verified after the change: 5289 tests pass, the fuzz target passes 3,000 cases, and |
Benchmarks43 regressions Highlights
Collections (566)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( |
marius-bughiu
added a commit
that referenced
this pull request
Aug 5, 2026
Picks up SortedSpan (#342), which landed on main while this PR was in review. Both changes add to the same set of shared surfaces, so all four conflicts were additive rather than contradictory: - CHANGELOG.md — both added [Unreleased] entries; kept both, Sorting first. - README.md — main added SortedSpan to the Celerity.Primitives row of the packages table while this branch added a Celerity.Sorting row below it; kept main's updated row and this branch's new one. - Celerity.Fuzz/Differential.cs — competing using directives; both are needed, since the file now drives SortedSpan and the three sorters. - web/index.html — competing ship cards; kept all four, with SortedSpan next to the other Celerity.Primitives entry and the three Celerity.Sorting cards after it. The auto-merged files were checked rather than assumed: the benchmark registry, both dashboard COLLECTIONS arrays, the fuzz target list and the AOT smoke test all carry both features, and neither side's ROADMAP status was clobbered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 #313.
SortedSpanships inCelerity.Primitives: merge-based set algebra over spans that are already sorted ascending.The BCL has no set operation over spans anywhere —
MemoryExtensionshas none,TensorPrimitiveshas none, .NET 10 added none — so the two things a developer writes today (HashSet<T>.IntersectWith, LINQIntersect) allocate a table and hash every element instead of using the order the data already has.The measured win (the issue's kill criterion)
1,000,000 x 1,000,000 sorted distinct
intspans over a 2,000,000 universe, ~50% overlap:HashSet<int>SortedSpanAllocation on that intersect: 17.9 MB (
HashSet) / 17.7 MB (LINQ) / 0 bytes (SortedSpan).Asymmetric 1k against 10M (the galloping shape): 0.37 ms vs 94.3 ms — 257x, and 422x for
IntersectCount. The kill criterion ("if the scalar merge does not beatHashSet<int>.IntersectWithat 1M elements, close the whole issue") clears with room to spare, so the scalar merge ships.Design calls worth reviewing
Vector256path, per the issue's own condition. The scalar merge is already memory-bound at 1M x 1M (~6 ms for one sequential pass over both inputs) and merge is branch-heavy enough that vectorizing it is frequently a wash; the >=25% bar was never plausible enough to justify a second implementation.IComparisonOperators<T, T, bool>, not hand-writtenint/long/uint/ulongoverloads. The JIT specializes the merge per value type and each comparison lowers to one instruction, so the issue's fallback was not needed. Floats compile but are documented as out of the intended use (NaNis not ordered under<).HashSet<T>differential oracle meaningful, and costs one predictable comparison per emitted element.Unionhas no galloping path andExceptgallops only when the subtrahend is the long side — in both excluded cases the result is proportional to the long input, so skipping comparisons cannot beat the cost of writing the answer out.ArgumentException(the alternative the issue offered was a negative sentinel). The shortfall is found while writing, so the destination's contents are then documented as undefined; the always-sufficient sizes (min(a,b)/a/a+b) are documented per method.AssertSortedis[Conditional("DEBUG")], so Release elides the call and itsO(n)argument evaluation — a Release check would cost exactly what the algorithm saves. It carries[ExcludeFromCodeCoverage]because its failing path callsDebug.Assert, which no test can drive without tearing down the test host.Parity rollout (all in this PR)
src/Celerity.Primitives/SortedSpan.csSortedSpanTests.cs— merge and galloping paths, both gallop directions, duplicate collapsing, empty/disjoint/identical/single-element, exact-fit and undersized destinations, non-intelement types, and aHashSetcross-check across five length ratiosSortedSpanDifferentialTests.cs(CsCheck: generated domain, independent side lengths so the ratio swings across the 32x threshold, both argument orders) and aSortedSpantarget inCelerity.Fuzz'sDifferential.AllSortedSpanBenchmark.cs, registered inCoreBenchmarksinProgram.cs;HashSet<int>baselines markedBaseline = trueper category, LINQ arms alongside,[MemoryDiagnoser],ItemCountsweep at 1,000 / 100,000, plus an asymmetric galloping rowCOLLECTIONSentry inweb/dev/bench/index.htmlandweb/dev/bench/detail.html(+Linqadded toBCL_TYPES), and a ship card onweb/index.htmldocs/api/utilities.mdsection with the measured table and the runnable example; README primitives paragraph, package-table row, and a "choosing a collection" row with the sortedness caveat stated in the row itself;Celerity.Primitives/README.mdand the packageDescriptionCelerity.AotSmokeTest/Program.cscovering the merge, the galloping path and the throw[Unreleased] / Addedbullets; the 2.4.0 roadmap item flipped fromplannedtodonewith the measurements and the three design calls recordedTwo checklist items from the issue do not apply and were not done: the shared cross-collection test suites (
AddAndTryAddTests,SetConstructorValidationTests, …) all parameterize over collection types, andSortedSpanis a static helper with no instance surface; and the coverage-gate dependency it named is already closed by #314, soCelerity.Primitivesis inside the gate and this code is measured.Test plan
dotnet buildclean, no new warnings.dotnet test— 5289 passed, 0 failed on each of net8.0, net9.0 and net10.0 (the CI matrix's three TFMs; net8.0/net9.0 run locally viaDOTNET_ROLL_FORWARD=Major).coverage.runsettings, cobertura).Celerity.Fuzz --target SortedSpan --iterations 3000— all cases pass against theHashSetoracle.node scripts/check_dashboard_coverage.jsandnode scripts/check_doc_anchors.js(+--self-test) pass.SortedSpanBenchmarkdry-run executed: all 30 result names parse with the dashboard's own regex, and all 12 (op, ItemCount) cards resolve to both a BCL and a Celerity series — so the card will not render blank.main, the benchmark workflow republishesdata.jstogh-pagesand the new SortedSpan card starts charting.🤖 Generated with Claude Code