feat(collections): add FenwickTree<T> — Binary Indexed Tree with O(log n) point update & prefix-sum query - #290
Merged
Merged
Conversation
…g n) point update & prefix-sum query FenwickTree<T> (where T : struct, INumber<T>) maintains a fixed-length numeric sequence and answers prefix / range sums and applies point updates both in O(log n), in one n-element array with no per-node overhead. It fills a BCL gap: there is no prefix-sum structure, and a plain array is O(n) per query or O(n) per update. The documented BCL-beating workload is any stream that interleaves updates with range-sum queries (running aggregates, rank / order-statistics counters, cumulative-frequency tables). Full parity rollout in one PR: - collection: src/Celerity/Collections/FenwickTree.cs - tests: FenwickTreeTests + FenwickTreeDifferentialTests (vs a naive long[] oracle) - benchmark: FenwickTreeBenchmark (Mixed + RangeSum vs long[]), registered in Program.cs CoreBenchmarks - dashboard: web/index.html ship card + both bench COLLECTIONS arrays - docs: docs/api/collections.md section + README (list, details block, decision table) - CHANGELOG: [Unreleased] bullet per facet The hash-table shared-test suites do not apply (numeric prefix-sum structure, like IndexedPriorityQueue / DisjointSet). Roadmap collection work is done through 2.1.0; this is the established post-roadmap tier-(c) enhancement pattern. Closes #289. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coverage
Files below 100% line coverage
|
Benchmarks2 regressions Highlights
Collections (420)
Hashers (100)
Same-runner A/B (sharded 8-way): main ( |
Resolves conflicts with the Trie (#286) and SparseSet (#288) merges, which touched the same shared parity files. Every conflict was a "keep both, in order" resolution — no content from either side was dropped: - Program.cs — CoreBenchmarks keeps Trie + Fenwick (SparseSet auto-merged) - web/index.html — ship cards for both Trie and FenwickTree - web/dev/bench/*.html — COLLECTIONS entries for both - README.md — both the "Prefix trees" and "Prefix sums" groups, both details blocks, and both decision-table rows (keeping main's updated iteration-order row that mentions Trie) - docs/api/collections.md — main's Trie section plus the FenwickTree section - CHANGELOG.md — all three Added entries plus main's new Fixed section Also condenses the FenwickTree changelog entry to a single user-facing bullet, matching the convention main just applied to the Trie and SparseSet entries. Full suite green after the merge: 4413 passed, 0 failed.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/Celerity/Collections/FenwickTree.cs:78
- The IEnumerable constructor also allocates
_treewithseed.Length + 1; ifseed.Length == Array.MaxLengththis will exceed the array ceiling. Add a guard with a clear exception before allocating.
T[] seed = values as T[] ?? values.ToArray();
_length = seed.Length;
_tree = new T[_length + 1];
src/Celerity/Collections/FenwickTree.cs:210
AddCorealways walks the tree and bumps_versioneven whendeltais zero. Early-returning ondelta == T.Zeroavoids pointless work and prevents no-op updates from invalidating enumerators.
for (int k = index + 1; k <= _length; k += k & -k)
_tree[k] += delta;
_version++;
…ersion bumps - Guard the construction length against Array.MaxLength - 1 (the 1-based layout reserves one slot), so an oversized length throws a clear ArgumentOutOfRangeException instead of overflowing into an OverflowException / OutOfMemoryException from the allocation. The IEnumerable ctor gets the matching ArgumentException guard. - A no-op update no longer bumps _version: AddCore returns early on a zero delta, which also covers the indexer setter (it reaches AddCore with `value - current`, zero exactly when the assigned value is already stored). This matches the rest of the library, where an operation that does not change the observable state does not invalidate active enumerators. It also skips the now-pointless O(log n) walk. - Fix the inaccurate baseline comment on the bench dashboard: for a plain array holding raw values, point updates are O(1) and prefix/range sums are O(n) — not O(n) for both. - Regression tests for all three, plus XML-doc and docs/api/collections.md updates so the no-op contract and the length ceiling are documented. Full suite green: 4417 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every sibling collection (Deque, DisjointSet, IndexedPriorityQueue, LruCache, SparseSet, ...) has a block in Celerity.AotSmokeTest; FenwickTree was missing one. It is the only collection built on generic math, so its INumber<T> static abstract members resolve through constrained calls the AOT compiler has to specialize per T — worth pinning under Native AOT rather than JIT only. The block exercises the O(n) seeded build, point update, prefix / range sums, the indexer round-trip, the no-op update path, clear-then-reuse and the struct enumerator, over two distinct T instantiations (long and int). Verified: "Celerity AOT smoke test: all checks passed." Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…line The bench dashboards split each benchmark's arms into a baseline/compare pair via parseName(), which marks an arm as the baseline only when its <TypeName>_<Op> type name is in BCL_TYPES. FenwickTree's baseline arms are named Array_* (a raw long[] — the BCL has no prefix-sum type, so that is the honest reference), and 'Array' was not in the set, so both Array_* and FenwickTree_* landed in the 'celerity' slot: no pair could form and the FenwickTree card would have rendered without a baseline or speedup. Adds 'Array' to BCL_TYPES in web/dev/bench/index.html and detail.html, with a comment explaining why the entry exists. Array_* is used only by FenwickTreeBenchmark, so no existing arm changes classification. web/index.html is deliberately untouched: its narrower BCL_TYPES feeds only the homepage headline, which aggregates Lookup/Contains/Insert/Add/Remove ops — FenwickTree's Mixed/RangeSum ops never reach it. Verified by replaying the dashboards' own parseName + pairing logic over the real method names: both FenwickTree ops pair correctly after the change and fail to pair before it, with DisjointSet unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…unted source The IEnumerable<T> constructor called ToArray() before enforcing the MaxLength ceiling, so an ICollection<T> whose Count exceeds the ceiling failed the allocation (OutOfMemoryException) instead of reporting the documented ArgumentException — and every counted source paid for an intermediate array that was then copied into the backing store. A counted source is now length-checked first and copied straight into the 1-based backing array via ICollection<T>.CopyTo(_tree, 1), which both fixes the exception and drops one full-size allocation and copy for the common T[] / List<T> case. Unknown-length sequences keep the materialize-then-check path. The `values as T[]` special case is subsumed — T[] is an ICollection<T>. Also corrects a stale comment in FenwickTreeDifferentialTests that referred to an "O(n) span build"; there is no span constructor, the long[] goes in through the IEnumerable<T> counted fast path. New tests cover the empty-source boundary (CopyTo targets index 1 of a length-1 array) on both paths, and a counted source that is neither T[] nor List<T> (SortedSet<int>) so the CopyTo path itself is exercised. Full suite green: 4419 passed, 0 failed. AOT smoke test: all checks passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hrow helper Replaces the hardcoded "values" literal in ThrowIfSourceTooLong with a paramName argument supplied via nameof at each call site, so a future rename of the constructor parameter cannot silently desync the ArgumentException's ParamName. No behavioural change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Celerity/Collections/FenwickTree.cs:256
AddCore’s update walk can overflowintfor large trees: whenkis a large power of two,k += k & -kcan wrap negative, causing an infinite loop (becausek <= _lengthstays true) or an invalid array access. Usinguint(orlong) for the loop index and low-bit math avoids overflow while keeping the array indexed byint.
for (int k = index + 1; k <= _length; k += k & -k)
_tree[k] += delta;
… overflow Both Fenwick ascents advance by adding the lowest set bit of the current 1-based index. At k == 1 << 30 the next index is 1 << 31, which overflows a signed int and wraps to int.MinValue — a negative value that still satisfies the `<= _length` bound, so the walk then indexed the backing array out of range and threw IndexOutOfRangeException. This is reachable, not theoretical: the length ceiling is Array.MaxLength - 1 (~2.1 billion), and the smallest INumber<T> is one byte, so a 2^30-element FenwickTree<byte> is about 1 GiB and allocates without gcAllowVeryLargeObjects. Widens the cursor in AddCore and the parent index in the O(n) build loop to long. Both walks still terminate at _length, so the casts back to int are always in range. The descending walks (PrefixSum / RangeSumCore) only ever strip bits and cannot overflow, so they are unchanged. Regression test Add_ShouldNotOverflowIndex_WhenTreeExceedsTwoToThe30 was confirmed to throw IndexOutOfRangeException against the pre-fix code and pass after. It is not skipped: the 1 GiB array is committed but never faulted in beyond the ~30 cells the ascent touches, so it runs in milliseconds. Add_LowestSetBitAscent_ShouldNotWrapAtTwoToThe30 pins the same arithmetic with no allocation at all. Suite green on net8.0 and net10.0: 4421 passed, 0 skipped. AOT smoke test passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mory Addresses the review concern that the >2^30 index-overflow regression test could be flaky on memory-capped CI environments. The test still builds a real 2^30-element tree, because that is the only thing that actually reproduces the bug: the overflow depends on _length, so no smaller instance reaches the failing step. (The arithmetic-only companion test that previously stood alongside it is removed — it re-evaluated the expression inline in the test file and never touched FenwickTree, so it could not fail regardless of the production code and offered no regression protection.) Instead of skipping unconditionally, the test now carries [MemoryIntensiveFact], a FactAttribute that consults GCMemoryInfo.TotalAvailableMemoryBytes at discovery time (which reflects the container/cgroup limit where one applies) and demands 3x headroom over the stated requirement. Environments with room run the check; constrained ones report it skipped rather than failing, so it can never turn the build red on resource grounds. Both paths verified: normal run -> executes and passes (~17 ms) DOTNET_GCHeapHardLimit=0x10000000 -> "Skipped! Failed: 0, Passed: 0, Skipped: 1" Suite green: 4420 passed, 0 skipped, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing, harden the test attribute
Review round 6:
- Extract the descending prefix walk into a single private PrefixSumCore, now
shared by PrefixSum, RangeSumCore, and the indexer getter. The bit-strip was
written three times across two methods; centralizing it removes the drift risk.
RangeSumCore becomes the obvious PrefixSumCore(end) - PrefixSumCore(start).
- Correct the "single n-element array" wording in the XML summary, the API
reference and the README: the 1-based layout means the backing array holds
n + 1 elements with index 0 unused. The substantive claim (one flat array, no
per-node object overhead) is unchanged.
- MemoryIntensiveFactAttribute now rejects a non-positive requiredMegabytes with
ArgumentOutOfRangeException — a non-positive threshold is meaningless and would
silently force the test to run everywhere, the exact behaviour the attribute
exists to prevent — and states the no-overflow intent with checked arithmetic.
- Tag the memory-intensive regression with [Trait("Category", "MemoryIntensive")]
so CI can segregate it (a serial job, or --filter "Category!=MemoryIntensive")
without it having to be opted out of by default.
Verified: full suite 4420 passed / 0 failed; trait exclusion drops exactly that
one test (4419); the memory gate still reports Skipped under
DOTNET_GCHeapHardLimit=0x10000000.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both callers have already established that the prefix bound is in range, so the public PrefixSum wrapper's check can never fail for them. MoveNext paid it once per element and kept the throw path reachable inside the loop body; Total's check compares _length against itself. Routing both at PrefixSumCore keeps every internal caller on the same rule. Behaviour is unchanged - covered by the existing enumeration, Total and differential tests.
Owner
Author
CI status: green except one benchmark shard, which was cancelled by the job timeout — not a benchmark failureEverything that gates correctness passed on
What actually happened. Shard 0's step timings (job):
So the PR-head measurements completed fine; the same-runner Why shard 0 specifically. Shard membership is greedy LPT bin-packing over the benchmark classes ( Impact is limited to this PR's comparison run. The other five shards passed and I have deliberately not touched |
…s the timeout The benchmark gate came back cancelled on this PR: `benchmark (shard 0)` hit the 120 min job cap. Its head slice took 65 min and the base slice was cancelled 55 min in; the other five shards ran 86-106 min total and passed. This is capacity, not a transient. Sharding by case count is the right metric -- BenchmarkDotNet targets a fixed iteration duration and scales invocations to reach it, so wall time per case is roughly constant regardless of how expensive one operation is. Adding a class therefore adds time in proportion to its case count, and at SHARD_TOTAL=6 the heaviest shard had no headroom left: even removing this PR's 8 cases entirely would have left it at ~119 min against a 120 min cap. main stays green only because push runs skip the base half, which hid how close the PR path had drifted to the limit. CiConfig.cs is explicit that the job schedule stays as-is and the matrix is what scales when the suite grows, so this raises SHARD_TOTAL 6 -> 8 (matrix 0..7) rather than trading away measurement accuracy. The same total work over 8 slices puts the heaviest back near ~95 min. The aggregate job already globs head-shard-*.json / base-shard-*.json and needs the whole matrix, so it picks up the two extra shards with no change. Also replaces the TRANSITIONAL note on the base step. It predicted that base runs would be cancelled until `--shard` landed on main and would then self-heal, but `--shard` is on main and the base tip honours it -- so the note was stale and actively misleading: it invites a reader to dismiss exactly this cancellation as expected. It now says a cancellation there is a real signal. Verified: the workflow parses, SHARD_TOTAL matches the matrix, and the matrix enumerates 0..N-1. Co-Authored-By: Claude Opus 5 <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.
Summary
Adds
FenwickTree<T>(Binary Indexed Tree) toCelerity.Collections— a fixed-length, array-backed numeric sequence that applies point updates and answers prefix / range sums both inO(log n), in onen-element array with no per-node overhead. Generic overSystem.Numerics.INumber<T>(int,long,uint,ulong,double,decimal, …).Closes #289.
The BCL gap / documented winning workload
.NET ships nothing for the interleaved point-update + prefix-sum-query workload, and a plain
T[]forces a losing tradeoff — keep the raw values and every prefix/range query isO(n)(point updates areO(1)); precompute a running-total array and queries areO(1)but every update isO(n). A Fenwick tree gives both inO(log n). It wins precisely when updates and partial-sum queries interleave: running aggregates, rank / order-statistics counters (inversions, "how many seen ≤ x"), cumulative-frequency tables, windowed sums over a mutating history.Parity rollout (all in this PR)
src/Celerity/Collections/FenwickTree.cs:intandIEnumerable<T>(O(n) build) constructors;Count/Total, indexer get/set,Add,PrefixSum,RangeSum,Clear; allocation-free struct enumerator with version-based mutation detection;IReadOnlyCollection<T>.FenwickTreeTests(core ops, indexer, constructors + validation + source-array non-aliasing,int/long/double, full enumeration surface) andFenwickTreeDifferentialTests(seeded randomized reconciliation of every value / prefix boundary / range query against a naivelong[]oracle).FenwickTreeBenchmark(Mixedinterleaved update+query stream andRangeSumbatch, vs a plainlong[]), registered inProgram.cs'sCoreBenchmarks.FenwickTreeblock inCelerity.AotSmokeTest, matching every sibling collection. This is the only collection built on generic math, so itsINumber<T>static abstract members resolve through constrained calls the AOT compiler must specialize perT; the block pins two instantiations (long,int) under Native AOT rather than JIT only.web/index.html+COLLECTIONSarrays inweb/dev/bench/index.htmlandweb/dev/bench/detail.html.docs/api/collections.md(with a runnable inversion-count example) + README (new "Prefix sums" group, a details block, and a decision-table row).[Unreleased], matching the brevity conventionmainapplies to theTrie/SparseSetentries.Parity items that genuinely do not apply: the cross-collection shared
Add/TryAdd/SetConstructorValidation/IEnumerableConstructor/EnsureCapacitysuites (this is a numeric prefix-sum structure, not a hash-table set/dict — same asIndexedPriorityQueue/DisjointSet), and there is noROADMAP.mdstatus to flip (collection work isdonethrough 2.1.0; this is the established post-roadmap tier-(c) pattern).Review follow-ups (all addressed)
Seven Copilot rounds; every thread replied to and resolved.
Round 1 (
747fd76)Array.MaxLength - 1(the 1-based layout reserves index0) with a clearArgumentOutOfRangeExceptioninstead of lettinglength + 1overflow; theIEnumerable<T>ctor gets the matching guard.AddCorereturns early on a zero delta, so neitherAdd(i, 0)nor assigning the value already stored bumps_versionor invalidates active enumerators (and both skip the pointlessO(log n)walk).long[]baseline, updates areO(1)and only the sums areO(n).0de3100; the per-facet detail lives here in the PR body.Round 2 (
20dc83f) — a real dashboard bug.parseName()classifies a benchmark arm as the baseline only whenBCL_TYPEScontains its type name, andArraywas missing, so bothArray_*andFenwickTree_*landed in thecelerityslot and the card would have rendered with no baseline or speedup.'Array'added toBCL_TYPESin bothindex.htmlanddetail.html.Round 3 (
1f77480,a06f467) — a real ordering bug.FenwickTree(IEnumerable<T>)calledToArray()before the length ceiling check, so an oversizedICollection<T>failed the allocation instead of reporting the documentedArgumentException. Counted sources are now length-checked first and copied straight into the 1-based backing array (no intermediateT[]); the throw helper also takes the parameter name.Round 4 (
0980e34,4532ec8) — a real overflow bug. Both ascending walks (AddCoreand theO(n)build) add the lowest set bit, so atk == 1 << 30the next index is1 << 31, which wraps negative in a signedint, still passes the<= _lengthguard, and then indexes out of bounds. Lengths that large are permitted (the ceiling isArray.MaxLength - 1), so this was reachable, not theoretical. Both cursors widened tolong. The regression test builds a 2^30-cellFenwickTree<byte>(~1 GiB committed, ~17 ms — the runtime zeroes lazily) behind a newMemoryIntensiveFactattribute, which reports the test skipped on a memory-capped runner rather than failing the build.Round 5 (
8a0aab9) — extractedPrefixSumCoreas the single unvalidated prefix walk, corrected the layout wording, hardened the new test attribute.Round 6 (
7f6fba1) — the enumerator called the publicPrefixSumonce per element even thoughMoveNext's own_index < _lengthguard already puts the bound in range, paying a redundant compare and keeping the throw path inside the loop body. It now callsPrefixSumCoredirectly. Applied the same reasoning toTotal(whose check compared_lengthagainst itself), soPrefixSumis left as the single validating entry point for user-supplied bounds and every internal caller is on the core.Round 7 — Copilot reviewed
7f6fba1and generated no new comments. No threads left open.0de3100also mergesmain(Trie #286 / SparseSet #288 landed while this was open); every conflict was a keep-both resolution in the shared parity files.CI: benchmark shard timeout
The benchmark gate came back cancelled on this branch —
benchmark (shard 0)hit the 120 min job cap (65 min head slice, base slice cancelled 55 min in). The other five shards ran 86–106 min and passed.Diagnosis, after measuring rather than assuming:
Array_*baseline arms were pathologically expensive per case (97 ms / 81 ms at 100k) and that the shard balancer, which weights by case count, could not see it. That was wrong. Cutting the per-op work 10× (arms down to 9.7 ms / 7.8 ms) left the class's wall-clock unchanged — 5m27s vs 5m38s — because BenchmarkDotNet scales invocations per iteration to hit a fixed iteration duration. Wall time tracks case count, not per-case cost, so the balancer's metric is correct and that change bought nothing. I reverted it.SHARD_TOTAL=6the heaviest shard had no headroom: even removing this PR's 8 cases entirely leaves it at ~119 min against a 120 min cap.mainstays green only because push runs skip the base half, which hid how close the PR path had drifted.Fix: raise
SHARD_TOTAL6 → 8 (matrix0..7).CiConfig.csis explicit that the job schedule stays as-is and the matrix is what scales when the suite grows, so this preserves measurement accuracy. The same work over 8 slices puts the heaviest back near ~95 min. The aggregate job already globshead-shard-*.json/base-shard-*.jsonand depends on the whole matrix, so it needs no change.Also replaced the
TRANSITIONALcomment on the base step: it predicted base runs would be cancelled until--shardlanded on main and would then self-heal, but--shardis on main and the base tip honours it. The note was stale and would invite a reader to dismiss exactly this cancellation as expected.Test plan
dotnet buildclean acrossnet8.0;net9.0;net10.0(0 warnings — XML-doc gate passes).dotnet test— fullCelerity.Testssuite green (4420 passed, 0 failed, on both thenet8.0floor andnet10.0), including the differential oracle and the new regression tests for the two behaviour fixes above.Celerity.AotSmokeTestruns clean locally ("all checks passed").aot-publishNative AOT matrix, and the coverage gate (line ≥ 95%, branch ≥ 90%).mainthe gh-pages dashboard refreshes with the newFenwickTreecard.🤖 Generated with Claude Code