Skip to content

Commit 8a0aab9

Browse files
marius-bughiuclaude
andcommitted
refactor(FenwickTree): extract PrefixSumCore, correct the layout wording, 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>
1 parent 4532ec8 commit 8a0aab9

5 files changed

Lines changed: 31 additions & 15 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ The mutable sets (`CeleritySet`, `SwissSet`, `RobinHoodSet`, `HashCachingSet`, `
8888

8989
**Prefix sums**
9090

91-
- `FenwickTree<T>` — a **Binary Indexed Tree** over a fixed-length numeric sequence (`where T : struct, INumber<T>`): **point update** and **prefix / range sum** both in `O(log n)`, in one `n`-element array with no per-node overhead. The prefix-sum structure the BCL lacks — running aggregates, rank / order-statistics counters, cumulative-frequency tables — where a plain array is `O(n)` per query (recompute the slice) *or* `O(n)` per update (fix the suffix). Wins precisely when updates and partial-sum queries interleave.
91+
- `FenwickTree<T>` — a **Binary Indexed Tree** over a fixed-length numeric sequence (`where T : struct, INumber<T>`): **point update** and **prefix / range sum** both in `O(log n)`, in one flat array with no per-node overhead. The prefix-sum structure the BCL lacks — running aggregates, rank / order-statistics counters, cumulative-frequency tables — where a plain array is `O(n)` per query (recompute the slice) *or* `O(n)` per update (fix the suffix). Wins precisely when updates and partial-sum queries interleave.
9292

9393
**Probabilistic & bit-level**
9494

docs/api/collections.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3660,7 +3660,7 @@ public sealed class FenwickTree<T> : IReadOnlyCollection<T>
36603660
where T : struct, INumber<T>
36613661
```
36623662

3663-
A **Fenwick tree** (Binary Indexed Tree) is a fixed-length, array-backed sequence of numeric values that answers **prefix sums** — and therefore arbitrary **range sums** — and applies **point updates** in `O(log n)` each, over a single `n`-element array with no per-node object overhead. It is generic over `System.Numerics.INumber<T>`, so it works for `int`, `long`, `uint`, `ulong`, `double`, `decimal`, and any other value type with generic-math addition and subtraction.
3663+
A **Fenwick tree** (Binary Indexed Tree) is a fixed-length, array-backed sequence of numeric values that answers **prefix sums** — and therefore arbitrary **range sums** — and applies **point updates** in `O(log n)` each, over a single flat array of `n + 1` elements (index `0` is unused by the 1-based layout), with no per-node object overhead. It is generic over `System.Numerics.INumber<T>`, so it works for `int`, `long`, `uint`, `ulong`, `double`, `decimal`, and any other value type with generic-math addition and subtraction.
36643664

36653665
The BCL 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 is `O(n)` (sum a slice); precompute a running-total array and queries are `O(1)` but every point update is `O(n)` (fix the whole suffix). A Fenwick tree gives **both** in `O(log n)`.
36663666

src/Celerity.Tests/Collections/FenwickTreeTests.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,10 @@ public void Enumerator_NonGeneric_ShouldYieldValues()
432432
// beyond the ~30 cells the ascent touches (the runtime zeroes lazily), so it completes in ~17 ms. Where
433433
// that headroom genuinely is not available (a memory-capped container or runner), MemoryIntensiveFact
434434
// reports the test skipped rather than running it, so it can never turn the build red on resource grounds.
435+
// The Category trait lets CI segregate this if it ever needs to — e.g. `--filter "Category!=MemoryIntensive"`
436+
// to exclude it, or a dedicated serial job to run it away from the parallel suite.
435437
[MemoryIntensiveFact(1024)]
438+
[Trait("Category", "MemoryIntensive")]
436439
public void Add_ShouldNotOverflowIndex_WhenTreeExceedsTwoToThe30()
437440
{
438441
const int length = 1 << 30; // 2^30 one-byte cells + the reserved 1-based slot ≈ 1 GiB

src/Celerity.Tests/MemoryIntensiveFactAttribute.cs

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,26 @@ public sealed class MemoryIntensiveFactAttribute : FactAttribute
1919
/// <summary>
2020
/// Marks a test as requiring <paramref name="requiredMegabytes"/> of allocatable memory.
2121
/// </summary>
22-
/// <param name="requiredMegabytes">The size of the allocation the test makes, in MiB.</param>
22+
/// <param name="requiredMegabytes">The size of the allocation the test makes, in MiB. Must be positive.</param>
23+
/// <exception cref="ArgumentOutOfRangeException"><paramref name="requiredMegabytes"/> is not positive.</exception>
2324
public MemoryIntensiveFactAttribute(int requiredMegabytes)
2425
{
25-
long required = (long)requiredMegabytes * 1024 * 1024;
26+
// A non-positive requirement would make the threshold meaningless and silently force the test to run
27+
// everywhere — exactly the behaviour this attribute exists to prevent. The argument is a compile-time
28+
// constant, so this fails the first time the test is discovered rather than at run time.
29+
if (requiredMegabytes <= 0)
30+
{
31+
throw new ArgumentOutOfRangeException(nameof(requiredMegabytes), requiredMegabytes,
32+
"The required size must be positive.");
33+
}
34+
35+
// Widening before the multiply already rules out overflow for every permitted argument; `checked`
36+
// states that intent rather than relying on the reader to re-derive it.
37+
long required = checked((long)requiredMegabytes * 1024 * 1024);
2638
long available = GC.GetGCMemoryInfo().TotalAvailableMemoryBytes;
2739

2840
// A non-positive reading means "unknown" — run the test rather than skip on missing information.
29-
if (available > 0 && available < required * RequiredHeadroomFactor)
41+
if (available > 0 && available < checked(required * RequiredHeadroomFactor))
3042
{
3143
Skip = $"Needs ~{requiredMegabytes} MiB of allocatable memory " +
3244
$"(with {RequiredHeadroomFactor}x headroom); this environment reports " +

src/Celerity/Collections/FenwickTree.cs

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ namespace Celerity.Collections;
66
/// <summary>
77
/// A <b>Fenwick tree</b> (Binary Indexed Tree): a fixed-length, array-backed sequence of numeric values that
88
/// answers <b>prefix sums</b> (and therefore arbitrary range sums) and applies <b>point updates</b> in
9-
/// <c>O(log n)</c> each, over a single <c>n</c>-element array with no per-node object overhead.
9+
/// <c>O(log n)</c> each, over a single flat array — <c>n + 1</c> elements, the slot at index <c>0</c> being
10+
/// unused by the 1-based layout — with no per-node object overhead.
1011
/// </summary>
1112
/// <typeparam name="T">
1213
/// The numeric element type. Constrained to <see cref="INumber{TSelf}"/>, so it works for <see cref="int"/>,
@@ -193,11 +194,7 @@ public T PrefixSum(int endExclusive)
193194
throw new ArgumentOutOfRangeException(nameof(endExclusive), endExclusive,
194195
"endExclusive must be in the range [0, Count].");
195196

196-
T sum = T.Zero;
197-
for (int k = endExclusive; k > 0; k -= k & -k)
198-
sum += _tree[k];
199-
200-
return sum;
197+
return PrefixSumCore(endExclusive);
201198
}
202199

203200
/// <summary>
@@ -266,18 +263,22 @@ private void AddCore(int index, T delta)
266263
_version++;
267264
}
268265

269-
// Range sum without validation: PrefixSum(endExclusive) - PrefixSum(start), collapsed to two walks.
270-
private T RangeSumCore(int start, int endExclusive)
266+
// The prefix walk, without validation — the single place the descending bit-strip is written, shared by
267+
// PrefixSum, RangeSumCore, and the indexer getter. Unlike the ascending walks this one only ever clears
268+
// the lowest set bit, so it strictly decreases and cannot overflow.
269+
private T PrefixSumCore(int endExclusive)
271270
{
272271
T sum = T.Zero;
273272
for (int k = endExclusive; k > 0; k -= k & -k)
274273
sum += _tree[k];
275-
for (int k = start; k > 0; k -= k & -k)
276-
sum -= _tree[k];
277274

278275
return sum;
279276
}
280277

278+
// Range sum without validation.
279+
private T RangeSumCore(int start, int endExclusive) =>
280+
PrefixSumCore(endExclusive) - PrefixSumCore(start);
281+
281282
private static void ThrowIfSourceTooLong(int count, string paramName)
282283
{
283284
if (count > MaxLength)

0 commit comments

Comments
 (0)