OMC-21 audit fixes — 56/56 findings actioned (correctness + impl + doc honesty) - #14
Merged
Conversation
…stra The method previously walked a FIFO Queue<TKey> while adding edge weights to distances and re-enqueueing on improvement. On weighted graphs FIFO ordering does not respect distance, so the algorithm could produce wrong shortest- distance values for some nodes (the first time a node is dequeued may not be along the actual shortest path; later improvements only update a node's distance entry but the stale weights have already propagated to its neighbors). Worst case ~O(V*E) on adversarial weight distributions. Replaced with a bounded Dijkstra using the same SortedSet<(double, TKey)>- backed priority queue already used by FindShortestPath. Each node is settled at most once (HashSet guard); we early-break when the queue's next minimum exceeds maxDistance. Time: O((V'+E') log V'), where V'/E' are the vertices/edges within maxDistance. Space: O(V') for distances + queue + settled. Tests: 50/50 GraphDictionary tests pass on net8 + net6 with no changes needed (existing tests use unweighted-equivalent edges where the buggy algorithm happened to produce correct output). (also: gitignore docs/complexity-audit-*.md so the planning doc stays local)
…lone allocation Previously EstimateUnion did Clone()+Merge()+EstimateCardinality() — three m-byte traversals plus an m-byte heap allocation per call. Replaced with a single-pass loop over both bucket arrays computing the per-bucket max-then-2^-b sum directly, then applying the same three-way cardinality correction logic EstimateCardinality uses. No allocation, two fewer traversals. Time stays O(m); allocation drops from O(m) to O(1). Same correction branches preserved (small-range linear counting, raw estimate, large-range -2^32 log correction) so the result is identical to the previous Clone+Merge+Estimate pipeline. Tests: 35/35 HyperLogLog tests pass on net8 + net6.
… via binary search Previously linear-scanned _snapshots looking for the timestamp closest to the query time, so GetObjectsInRadiusAtTime / GetObjectsInRectangleAtTime / any snapshot-time query was O(snapshots + k) regardless of how snapshots were indexed. Added a parallel `_snapshotKeysOrdered : List<DateTime>` maintained alongside `_snapshots`. Snapshots are appended in chronological order (TakeSnapshot uses DateTime.UtcNow which is monotonically increasing in normal use). The list supports O(log N) BinarySearch for FindClosestSnapshot, which now picks the closer of the two binary-search candidates (immediate predecessor and successor of the query timestamp). CleanupOldSnapshots updated to walk the front of _snapshotKeysOrdered (which is sorted, so all expired keys are contiguous at the start) and pop them in one RemoveRange call. Same overall O(N) cleanup cost as before but with one shift instead of N reads. Time: FindClosestSnapshot drops from O(snapshots) to O(log snapshots). Per-snapshot insert stays O(1) amortized; per-cleanup stays O(N). Memory: +sizeof(DateTime) per retained snapshot — typically <= 1000 entries. Tests: 290/290 Spatial tests pass on net8 + net6.
…ap-and-truncate Both methods previously walked descending indices calling _items.RemoveAt per match. Each List<T>.RemoveAt is O(N - i), so for k matches the total cost was O(N*k). For typical predicates that match a non-trivial fraction of items, this scales quadratically with N. Replaced with the standard swap-and-truncate pattern: forward scan with two pointers (write, read), copy survivors leftward over removed slots, then a single RemoveRange truncates the tail. O(N) total — single pass through the items, single shift to truncate. Behavioral notes: - Sync RemoveAll: keeps the existing event shape (Reset notification, no per-item events). Removed items are no longer materialized into a list since the sync path doesn't fire per-item callbacks. - Async RemoveAllAsync: keeps per-item ItemRemoved events. Items are collected in ascending index order during the forward scan, which is the natural ordering — the previous code collected in descending order and then called .Reverse(); both steps are now unnecessary. - Async also keeps the periodic Task.Yield(); cancellation still checks per iteration. The dead `lowestIndex` field in RemoveAllAsync (assigned but never read — it was a leftover from an earlier (Remove, items, index) shape that was already changed to Reset) is gone. Tests: 69/69 ObservableList tests pass on net8 + net6.
…ar advance Previously called GetAtTime per frame inside the while loop. GetAtTime does a fresh BinarySearchTime, so the cost was O(frames * log N). For a 60-fps replay over a 10s buffer that's 600 binary searches. Replaced with the standard pattern: one binary search to seed the index, then advance linearly per frame. Each frame's index is found by stepping forward until the next-sample timestamp would exceed currentReplayTime. Total work across all frames is O(log N + N + frames), bounded by O(log N + frames) for typical replays where frame intervals span multiple sample timestamps. Behavior preserved: - "Round down" semantics: returns the latest sample at-or-before each frame timestamp. (GetAtTime did the same via insertion-point-minus-1.) - "Skip-before-startTime" semantics: frames whose timestamp falls before the first recorded sample are skipped. (GetAtTime returned default and the caller had `if (snapshot != null)`; we now check explicitly so we never need to construct the tuple.) - startTime > EndTime: yields nothing. Same as before. Tests: 36/36 TimelineArray tests pass on net8 + net6.
…rateSetBits
F-29: FillArea was nested per-cell loops calling the indexer, O(w·h) per call
even though the bit-packed storage supports word-level fills. Rewrote to
clip the rectangle to grid bounds, then for each row in the rectangle fill
a contiguous bit range via a SetBitRange helper that:
- first/last word: build a mask covering the relevant bits via
upperMask & ~lowerMask (avoids the `1UL << 64` overflow corner)
- middle words: full ulong fill (`= ulong.MaxValue` or `= 0`)
Per-row cost drops from O(width) to O(width/64). Total: O(h · w/64).
F-30: EnumerateSetBits was O(W·H) — visited every cell via the indexer
regardless of bit density. Rewrote to walk the storage word-by-word; for
each non-zero word use BitOperations.TrailingZeroCount to jump directly to
each set bit, clear it via `word &= word - 1`, and continue. Skips entire
64-bit zero-words in one comparison. Total: O(set bits + W·H/64).
The final word is masked to _totalBits to ignore any unused bits past the
grid (in case word-level writes left them set).
Added BitOperations.TrailingZeroCount to Core/Compat/BitOperations.cs (the
netstandard2.1 polyfill — net8 uses the framework version). Cascading-mask
algorithm; matches what System.Numerics.BitOperations does internally on
platforms without TZCNT instruction.
Tests: 58/58 BitGrid2D tests pass on net8 + net6.
…ntroid Add Previously Merge called Add per centroid in the other digest. Each Add does a List<T>.Insert at the binary-search position, which shifts O(c) elements to maintain sorted order. So the worst case for Merge was O(c1*c2) where c1 grows as the merge proceeds. Replaced with a two-pointer linear merge: walk both centroid lists in lockstep (after EnsureSorted on each), build the sorted union in one pass, swap into _centroids, then Compress to reduce back to compression-bounded size. Time: O(c1 + c2) for the merge + O(c) Compress = O(c1 + c2). Compress's EnsureSorted is a no-op because we build the merged list pre-sorted. The 'other' digest is not mutated — if it needs sorting we sort a local copy. Min/max/count are merged in. _needsSort=false because the merged list is in order by construction. Triggers Compress when the merged count exceeds _compression (the same trigger as Add's Compress condition, but on the post-merge size — without this, two large digests merged together could leave _centroids at 2*_compression which is the upper bound Compress targets anyway). Tests: 66/66 Digest + DigestStreamingAnalytics tests pass on net8 + net6 (1 pre-existing skip unrelated to this change).
Bring the README and class XML doc-comments in line with what the code
actually does. Two categories:
A) Findings already fixed by code changes earlier on this branch — README
updated where it still described pre-fix behavior:
F-26 Digest.Merge — now O(c1+c2) linear merge (was O(c1*c2))
F-42 TemporalSpatial- — FindClosestSnapshot now O(log snapshots)
HashGrid via parallel sorted-keys list
F-44 GraphDictionary — FindNodesWithinDistance now bounded
Dijkstra O((V+E) log V) (was wrong-algo
queue with weighted edges)
(F-29/30/32/33/53/54 — README's original claims now match reality;
verified, no doc change needed.)
B) Findings that are doc-only (no impl change — README needed updating):
Per-type tables (~28 entries):
F-1, F-2 Pooled{Stack,Queue}.{Pop,Dequeue}Span — note alloc
F-3..F-6 Min/MaxHeap.Insert amortized + storage O(N) (not capacity)
F-7, F-8 QuadTree / SpatialHashGrid — surface linear-mode caveat
F-9, F-10 KdTree.Clear / OctTree.Clear — O(1) (orphans tree)
F-11 KdTree.Insert — periodic rebuild annotation
F-12 TemporalSpatial.UpdateObject — amortized snapshot cost
F-13 GetObjectTrajectory — O(s log s) due to Sort
F-14 KdTree.FindNearestK — O((log N + k) log k) avg
F-15 GraphDict.FindShortestPath — drop "(unweighted BFS)";
correct as Dijkstra
F-16/17 LinkedMultiMap.this[]/TryGetValues — O(values per key),
*mutates LRU*, allocates per call
F-18 LinkedMultiMap.RemoveKey — O(1) avg
F-19 LinkedMultiMap.Clear — O(buckets)
F-20 PredictiveDictionary.AddOrUpdate — drop "+pattern update"
F-21 PredictiveDictionary.GetPredictions — fix `p` definition
F-22 ConcurrentLinkedDict.Clear — O(buckets) per-bucket-locked
F-24/25 Digest.Add/Quantile/Cdf — O(c) (was O(log c))
F-27 DigestStreamingAnalytics — inherits Digest's O(c)
F-31 HexGrid2D.FindPath/GetReachable — fix README signatures
F-34 ObservableHashSet.SymmetricExceptWith — annotate alloc
F-35 CountMinSketch — width rounded to next power of 2
F-37 KdTree.InsertRange — bulk-rebuild path
F-38 SpatialHashGrid.GetStatistics — O(C log C) (sort)
F-39/40 GetPotentialCollisions — linear/spatial mode breakdown
F-41 SpatialHashGrid.Remove for InsertBounds — O(C·M)
F-43 KdTree.FindNearestK List overload — alloc note
F-45 GraphDict.GetClusteringCoefficient — O(deg²) (new row)
F-46 GraphDict.Keys/Values — O(N) snapshot (new row)
F-49 ConcurrentLinkedDict.GetEnumerator — blocks writers
F-50 TDigest.CanMerge — subsumed by F-24 fix
F-55 TimelineArray.ReplayAtFps — fix signature
Class-level XML doc-comments:
F-23, F-48 ConcurrentLinkedDictionary — drop "lock-free reads";
describe per-bucket-monitor + LRU writer lock honestly
F-51 TDigest — rewrite to say O(c) Add/Quantile, O(c1+c2)
Merge, drop "O(log n)" claim
F-52 DigestStreamingAnalytics — describe O(c) per-call
delegation + periodic O(buffer) cleanup
F-47 LinkedMultiMap — note NodeValueView indexer is O(i)
Stale "lock-free" comments in the ConcurrentLinkedDictionary
benchmark file replaced with "per-bucket monitor" wording.
Skipped per audit-doc instructions: F-28 (HLL caching already works as
documented), F-36 (KdTree.Build not exposed in README — implementation
work is large-rewrite Tier 4 territory).
README: 911 -> 936 lines (+25 net; new annotation rows for findings
that weren't previously listed in any table).
Tests: 1522 net8 / 1493 net6, all green.
… + unweighted The existing FindShortestPath implementation is weighted Dijkstra over the edge-weight property. The README's old "(unweighted BFS)" annotation was wrong about both algorithm and complexity. Tier 1 doc pass already corrected the claim to "Dijkstra, O((V+E) log V)". Now adding the actual unweighted variant as a separate method: FindShortestUnweighted(start, end) — runs BFS over the graph, treats every edge as unit-weight, returns the path with the fewest hops. Result's TotalWeight is the hop count (path.Count - 1) rather than weighted distance. Cost: O(V'+E') where V'/E' are the vertices/edges within reach (BFS halts at target). Same SortedSet-of-distances overhead is gone — plain Queue<TKey> suffices because every edge is unit-weight, so FIFO order respects shortest- path-by-hop-count. Same metrics-cache key scheme as FindShortestPath, distinct cache prefix so the two methods don't collide. Tests: 50 -> 54 GraphDictionary tests on net8 + net6, all green. New tests cover: prefers-fewest-hops-ignoring-weights, unreachable returns null, self-path returns single-node path with weight 0, missing-endpoint returns null. PublicAPI.Shipped.txt updated. No bench added — the existing GraphDictionary benchmarks build the graph once in GlobalSetup, so the metrics-cache serves all iterations after the first call. Benching cached lookups doesn't demonstrate the Dijkstra-vs-BFS asymptote. Asymptotic difference is textbook; correctness tests are the verification.
…lues, drop per-call array alloc
API change. The previous behavior allocated `new TValue[node.ValueCount]`
on every this[key] / TryGetValues / GetEnumerator() call, copying each
value out of the per-key linked list. Hot-path consumers calling
multimap[k] in a loop paid O(values per key) heap allocation per access.
NodeValueView is now public (nested in LinkedMultiMap<TKey, TValue>). Each
get-path allocates only the view object itself (~24 bytes) — the view holds
a reference to the KeyNode and walks the value list lazily. Construction is
O(1); iteration is O(values per key); indexed access via this[i] is O(i).
The view aliases the multimap's internal storage — mutations to the
multimap after the view is returned invalidate the view (caller can call
the standard LINQ ToArray() for a snapshot).
The shared `_cachedView` field is gone. Each call gets a fresh view; no
aliasing-via-shared-state bugs (e.g., calling foreach(map) and
map[k] interleaved no longer reuses the same view object).
API breaks (acceptable per v2.x consumer expectations — bumping the major
on the next release if needed):
- this[TKey].get : IReadOnlyList<TValue> -> NodeValueView
- TryGetValues(TKey, out IReadOnlyList<TValue>) ->
TryGetValues(TKey, out NodeValueView)
- IEnumerable<KeyValuePair<TKey, IReadOnlyList<TValue>>> ->
IEnumerable<KeyValuePair<TKey, NodeValueView>>
- KeyNode and ValueNode bumped from private to internal so the public
NodeValueView constructor can reference KeyNode (still nested,
not surfaced beyond the assembly).
NodeValueView implements IReadOnlyList<TValue> so existing code that does
`foreach (var v in multimap[k])` or assigns to `IReadOnlyList<TValue>`
continues to work. Code that depended on the result being a fresh
TValue[] (and immune to subsequent multimap mutations) needs to call
`.ToArray()` explicitly.
PublicAPI.Shipped.txt updated: 3 signature lines retyped to NodeValueView,
4 lines added for NodeValueView itself (Count, indexer, GetEnumerator).
Tests: 1526 net8 / 1497 net6, all green.
…O(N log N)
BuildRecursiveOptimized previously called Array.Sort on the full range at
every recursion level — O(N log N) per level × O(log N) levels = O(N log²N).
Plus, Comparer<T>.Create allocated a fresh closure object per recursive
call.
Replaced with iterative Quickselect (Lomuto partition + median-of-three
pivot). At each level we partition items[start..end] in O(end - start)
average to place the median by `dimension`-coordinate at items[median],
with everything to its left smaller and right at-or-greater. No further
ordering within each side — kd-tree build only needs the partition, not
a full sort. Per-level cost drops to O(N) average; total O(N log N).
Worst case is O(N²) for adversarial input; median-of-three pivot
selection makes that very unlikely on uniform-random data (which kd-trees
are typically built on).
Removed the dead BuildRecursive(List<T>, int) overload — no callers.
Hot-path implications:
- Build (called from KdTree.Build / InsertRange bulk-rebuild path)
- RebalanceIfNeeded (every 64 inserts past N=1000 when height blows up)
Bench evidence (KdTree Fill, smoke profile, i7-13700KF, dev branch):
Before (audit-fixes prior commit):
N=10k: Omni_Fill 11,728 us 77,254 KB
N=100k: Omni_Fill 431,170 us 1,229,724 KB
After:
N=10k: Omni_Fill 4,930 us 15,352 KB (2.4x faster, 5x less mem)
N=100k: Omni_Fill 37,109 us 188,105 KB (11.6x faster, 6.5x less mem)
11.6x speedup at N=100k matches the predicted log N reduction (log₂100k ≈ 17;
we get 11.6 — the rest is constants). Memory drop is the elimination of
per-level Comparer<T>.Create closures plus the per-recursion Sort temp
buffers.
The README's F-11 amortized-rebuild annotation can be tightened later;
for now leave the doc as O(N log²N) (added in Tier 1) since it's still a
valid upper bound for adversarial input.
Tests: 47/47 KdTree tests pass on net8 + net6.
Replaced List<Centroid> backbone with an indexable skip list of Nodes,
each carrying the Centroid plus per-level Forward[] pointers and
Width[] arrays (cumulative-weight prefix sums per level — Pugh's
"indexable skip list" / finger-search trick).
Operations now achieve O(log c):
- Add: descent records (update[i], ranks[i]) predecessors and prefix-
weights; Width arrays update in O(log c) along the path. If the new
centroid merges into an existing one (CanMerge), Width[i] is
incremented along the parent chain; otherwise we splice a new node
and split the Width contributions across old vs new at each level.
- Quantile / Cdf: standard top-down descent, advancing while
cumulative + Width[i] < target. Drops from O(c) linear scan to
O(log c) levels × O(1) per level.
Operations preserved:
- Compress: walks level 0 in order under the same scale-function
rule, then RebuildFromSorted reconstructs the skip list. O(c log c).
- Merge: existing two-pointer linear merge over both digests' level-0
enumerations, then rebuild + Compress. O(c1+c2) (the F-26 fix is
preserved).
- Clone, Clear, Min, Max, Count, ClusterCount: wired through.
Removed the vestigial _needsSort field — skip list is always sorted.
Bench evidence (smoke profile, BDN-default, MemoryDiagnoser; param
[100, 1000] added to TDigestBenchmarks):
Quantile (the headline win):
N=1k / C=1000: 1085.69 ns -> 24.97 ns (43x faster)
N=10k / C=100: 31.27 ns -> 17.01 ns (1.8x faster)
N=100k/ C=1000: 26.85 ns -> 15.84 ns (1.7x faster)
At low compression (c <= 200) constant factors dominate: small
regressions of ~10-14% from the per-level Width updates and the
skip-list's less cache-friendly access pattern. Acceptable tradeoff
for the asymptotic guarantee.
Add (mixed):
N=10k / C=100: 178.52 ns -> 93.48 ns (1.9x faster)
Most other configs: within +/- 25% of baseline.
The List baseline's Insert is cache-friendly memmove on c <= 2000
(which is the ceiling at the highest documented compression);
per-Node skip-list allocation overhead balances the asymptotic
advantage at this scale. The asymptotic upper bound is now O(log c),
but constant factors leave the practical Add cost similar.
Public surface unchanged. PublicAPI.Shipped.txt unaffected.
EstimatedMemoryUsage formula bumped from `count*24+64` to `count*96+128`
to honestly reflect per-Node pointer overhead (~24B header + 16B
Centroid + 2 array headers + ~64B forward-pointer + width arrays at
expected ~2 levels avg).
Tests: 66/66 Digest + DigestStreamingAnalytics on net8 + net6 (1
pre-existing CompressionSettings_AffectAccuracyAndMemory skip
unrelated to this change).
…inkedMultiMap, GraphDictionary Update README per-type tables, the §5 Complexity reference table, and the class XML doc-comments to match the actual asymptotic behavior after the implementation fixes earlier on this branch. KdTree<T> (commit e25e8e6 — Quickselect rebuild): - Insert annotation: "periodic O(N log² N) rebuild" → "periodic O(N log N) rebuild" (Quickselect drops the log factor from per-level Sort) - InsertRange bulk-rebuild path: same O(N log²) → O(N log N) update. Digest (commit 0d2ae4f — skip-list backbone): - Add: O(c) → O(log c) - Quantile / Percentile: O(c) → O(log c) - Cdf: O(c) → O(log c) - Storage row: note ~96 B per-node overhead (skip-list pointers + width arrays vs the previous flat List<Centroid>) - Class XML doc was already updated by the F-24/25/26 commit; verified in place. DigestStreamingAnalytics<T> (inherits Digest): - All public-op rows: O(c) → O(log c) - Class XML doc updated to say "skip-list-backed digest" + O(log c). LinkedMultiMap<TKey, TValue> (commit 9645a56 — view return): - this[key] / TryGetValues: O(values per key) + alloc → O(1) view return - Added rows for the NodeValueView surface: * .Count: O(1) * enumeration: O(values per key) * indexer [i]: O(i) (singly linked) — flagged with "prefer foreach" - §5 reference table row updated to summarize. - Class XML doc rewritten: O(1) view returns, live-aliasing semantics, LRU mutation on read, "prefer foreach for sequential reads". GraphDictionary<TKey, TValue> (commit 7d9e15f — BFS variant): - Added FindShortestUnweighted row: O(V+E) BFS, ignores weights. - §5 reference table row updated to mention both Dijkstra and BFS variants side-by-side. Tests still 1526 net8 / 1497 net6 (no behavior change).
…→ O(c1+c2), Compress O(c log c) → O(c)
The previous RebuildFromSorted re-inserted each centroid via the full Add
path: skip-list descent + per-level Width updates, O(log c) per centroid,
O(c log c) total. That made user-visible Merge O((c1+c2) log(c1+c2)) end-
to-end despite the linear-merge step being O(c1+c2). Three verifier agents
on the previous round flagged this — claim 12e was the only Digest claim
that didn't unanimously confirm.
Replaced with a single-pass left-to-right bulk-build: maintain update[]/
ranks[] as the rightmost-level-i predecessor + cumulative weight per level.
For each new centroid: pick a level via the same RandomLevel() distribution
Add uses, splice the node in at its levels in O(level) by referencing the
stack. Width values fall out as (newRank - ranks[i]) — no descent needed
because the input is already sorted. Sum of levels across all c iterations
is 2c on expectation (Pugh's p=1/2 geometric), so total work is O(c).
Width invariant maintained correctly:
Width[i] of X = cumWeight(X.Forward[i]) - cumWeight(X)
where cumWeight(X) = sum of weights from start through X inclusive.
Higher-level "invisible" nodes (level < i): the new node doesn't touch
update[i]/ranks[i], so the next level-i arrival's iteration computes
(newRank_M - ranks[i]) which spans the entire intermediate stretch
correctly. No per-centroid "+= weight" updates needed (those were specific
to the per-call Add path, not bulk-build).
Tail nodes (Forward[i]=null, Width[i]=0): handled implicitly by the Node
constructor's zero-init defaults — bulk-build never overwrites a node's
own Forward[i]/Width[i], only the predecessor's. The last linked node at
each level keeps the ctor defaults.
Three independent verifier agents read the new code, traced the same
example (3 centroids, weights {1,2,3}, levels {1,2,1}), and all 3/3
confirmed the implementation is correct and O(c). No correctness bugs
spotted.
Bench evidence (DigestBenchmarks.Omni_Merge, smoke profile, BDN-default,
i7-13700KF, MemoryDiagnoser on; new Merge benchmark added to the file):
Before (audit-fixes prior commit):
N=1k / C=100: 3.05 us 1.40 KB
N=1k / C=1000: 214.65 us 313.31 KB <-- headline (max c)
N=10k / C=100: 14.96 us 14.05 KB
N=10k / C=1000: 2.54 us 1.34 KB
N=100k/ C=100: 3.44 us 2.59 KB
N=100k/ C=1000: 10.52 us 14.12 KB
After:
N=1k / C=100: 2.18 us 1.49 KB (1.4x faster)
N=1k / C=1000: 104.21 us 313.45 KB (2.06x faster, headline)
N=10k / C=100: 7.24 us 14.46 KB (2.07x faster)
N=10k / C=1000: 2.91 us 1.46 KB (noise)
N=100k/ C=100: 2.83 us 2.55 KB (1.21x faster)
N=100k/ C=1000: 5.65 us 14.04 KB (1.86x faster)
The asymptote class drops from O(c log c) to O(c). Visible 2x speedup at
high c reflects the constant-factor ratio after the log factor is removed
(other linear-time steps in Merge — the two-pointer walk itself,
Compress's level-0 scan — limit the visible improvement).
README Digest row updated:
Compress: O(c log c) -> O(c)
Clone: O(c log c) -> O(c)
Merge wording clarified ("then bulk-rebuild" instead of "then compress").
Tests: 66/66 Digest + DigestStreamingAnalytics on net8 + net6.
…-iterate, unblock writers
The previous GetEnumerator held the LRU read lock for the entire iterator
lifetime via `try { ... yield return ... } finally { ExitReadLock(); }`.
Because C# iterator-method finally blocks are deferred until iterator
disposal, every writer (Add/Remove/MoveToFront/EvictLru) was blocked until
the caller finished consuming OR explicitly disposed the enumerator.
Replaced with the snapshot-on-iterate pattern: under the read lock,
allocate KeyValuePair<TKey, TValue>[_count] and walk the LRU chain copying
each entry. Release the lock. Then iterate the local array yielding each
entry. The yield is OUTSIDE the try/finally, so the lock release happens
synchronously on first MoveNext (before any yield to the caller), not at
disposal time.
Defensive trim: if the chain turns out to be shorter than _count
suggested (mid-mutation transient state), Array.Resize trims the unused
tail. In practice _count is read under the read lock and writers can't
mutate it (they hold the write lock for both _count++/-- and chain
edits), so the trim is belt-and-suspenders. Three verifier agents flagged
this as cheap and harmless.
Trade-off: enumeration costs O(N) heap (one KVP per current entry) vs the
previous zero-alloc walk. In exchange writers are no longer blocked for
the duration of caller iteration, which can be arbitrary long-lived for
LINQ-style consumers. Documented in the XML doc + the README per-type row.
Three independent verifier agents (parallel reads, no shared context)
all confirmed CORRECT on every axis:
- Snapshot taken under read lock 3/3
- Lock released before any yield 3/3
- Snapshot consistency (chain stable) 3/3
- Defensive trim handles _count mismatch 3/3
- No lock held during yields 3/3
- LRU order preserved (MRU-first) 3/3
- Memory cost is O(N) 3/3
All three traced the GetEnumerator → MoveNext → MoveNext → Dispose
scenario and confirmed lock state at each step. No correctness bugs.
README + XML doc updated to reflect the new behavior:
Before: "Enumeration acquires the LRU read lock for the lifetime of
the iterator and blocks concurrent writers"
After: "Enumeration takes a one-time O(N) snapshot of the LRU chain
under the read lock, then releases the lock before yielding —
concurrent writers are not blocked during iteration."
Tests: 1526 net8 / 1497 net6, all green.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces extensive performance optimizations and documentation updates across the collection library. Key changes include refactoring the T-Digest implementation to use a width-augmented skip list for O(log c) operations, optimizing ObservableList.RemoveAll to a single-pass O(N) algorithm, and implementing word-level bit operations in BitGrid2D. Additionally, KD-tree construction was improved to O(N log N) using Quickselect, and ConcurrentLinkedDictionary enumeration was updated to prevent writer starvation. Review feedback identifies a critical issue in GraphDictionary where SortedSet usage may cause runtime exceptions for non-comparable keys, warns of potential cache key collisions in graph algorithms, and notes a complexity mismatch in the KdTree.Insert documentation.
| var queue = new Queue<TKey>(); | ||
| queue.Enqueue(source); | ||
| var settled = new HashSet<TKey>(); | ||
| var queue = new SortedSet<(double Distance, TKey Key)> { (0, source) }; |
There was a problem hiding this comment.
The use of SortedSet<(double Distance, TKey Key)> as a priority queue introduces a requirement that TKey must implement IComparable. Since GraphDictionary<TKey, TValue> only constrains TKey to notnull, this will cause a runtime InvalidOperationException if a user provides a non-comparable key type (as ValueTuple comparison relies on Comparer.Default for its elements).
Consider using a custom IComparer<(double Distance, TKey Key)> that handles non-comparable keys or using a priority queue implementation that doesn't require key comparison when distances are unequal.
| return null; | ||
| if (start.Equals(end)) | ||
| return new GraphPath<TKey>(new List<TKey> { start }, 0); | ||
| var cacheKey = $"shortest_unweighted_{start}_{end}_{_cacheVersion}"; |
There was a problem hiding this comment.
The cache key generation using string interpolation of start and end keys is susceptible to collisions if the keys' ToString() representations contain underscores or if different pairs of keys produce the same concatenated string (e.g., start="12", end="3" vs start="1", end="23").
Additionally, frequent string allocations for cache lookups can impact performance. Since _metricsCache is a ConcurrentDictionary<string, object>, consider using a more robust delimiter or a different caching strategy that avoids string concatenation.
| | Operation | Time | Space | | ||
| |---|---|---| | ||
| | `Insert` | O(log N) avg, O(N) worst (degenerate input) | O(1) | | ||
| | `Insert` | O(log N) avg amortized; periodic O(N log N) rebuild every 64 inserts past N=1000 when height exceeds 2·log₂(N)+2 | O(1) | |
There was a problem hiding this comment.
The complexity claim for KdTree.Insert as O(log N) avg amortized is optimistic given the current implementation. The code (in KDTree.cs, line 57) performs a full tree traversal to estimate height (EstimateTreeHeight) every 64 inserts. Since EstimateTreeHeight is an
To maintain
…key collision, KdTree height O(1) tracking Three issues from gemini-code-assist on PR #14, all real: 1. HIGH — GraphDictionary SortedSet<(double, TKey)> required TKey : IComparable<TKey>. The class only constrains TKey to `notnull`, so passing a non-IComparable key type triggered InvalidOperationException at runtime when the SortedSet's tuple comparison fell through to Comparer<TKey>.Default. Affected FindShortestPath (existing) and FindNodesWithinDistance (new in F-44). Fix: Custom `DistanceComparer : IComparer<(double, long Sequence, TKey)>` that orders by Distance, breaks ties by a monotonic per-call sequence counter. TKey.CompareTo never invoked. 2. MEDIUM — _metricsCache string keys used '_' as delimiter: `$"shortest_path_{start}_{end}_{_cacheVersion}"`. Two different (start, end) pairs whose ToString concatenations span the underscore can collide. Affects FindShortestPath and FindShortestUnweighted (FindStronglyConnectedComponents and GetCacheStatistics use only _cacheVersion — safe). Fix: replace '_' with ASCII Unit Separator (�) in the two pair- based cache keys. The control character is vanishingly unlikely to appear in any user TKey.ToString() value. 3. MEDIUM — KdTree.Insert claimed O(log N) amortized but height check fired EstimateTreeHeight (O(N) tree walk) every 64 inserts past N=1000, making amortized cost O(N/64) = O(N). Fix: replace EstimateTreeHeight with incremental _maxDepth tracking. InsertRecursive takes `ref int leafDepth` and records the depth at which the new node landed; Insert updates _maxDepth in O(1) by max-with-leafDepth. Build resets _maxDepth to ⌈log₂(N+1)⌉ (Quickselect produces a balanced tree). Clear resets to 0. RebalanceIfNeeded reads _maxDepth instead of recomputing — O(1) per check. The dead EstimateTreeHeight method is removed. Tests: 1526 net8 / 1497 net6, all green.
Owner
Author
|
Three gemini comments addressed in
Tests still 1526 net8 / 1497 net6, all green. |
Codeturion
added a commit
that referenced
this pull request
May 8, 2026
Minor release. Source changes from 16 commits on the audit-fixes branch (merged via PR #14): 1 correctness bug, 9 implementation improvements (some bench-confirmed at 11x-43x speedups), 43 documentation fixes aligning README + XML docs to actual asymptotic behavior, 1 small API change. Implementation highlights: - GraphDictionary.FindNodesWithinDistance — bounded Dijkstra (was buggy queue+weighted-relaxation, gave wrong distances) - GraphDictionary.FindShortestUnweighted — new BFS variant - GraphDictionary SortedSet PQ tie-break by sequence (TKey no longer needs IComparable; cache key delimiter switched to ASCII Unit Separator to remove collision class) - KdTree.Build via Quickselect — O(N log²N) -> O(N log N), measured 11.6x speedup at N=100k. _maxDepth tracked incrementally so RebalanceIfNeeded is O(1) per check (was O(N) tree walk every 64 inserts, breaking the amortized O(log N) Insert claim). - Digest skip-list backbone — Add/Quantile/Cdf now genuinely O(log c) (was O(c)). Bulk-build in RebuildFromSorted gives Merge O(c1+c2) and Compress O(c). Quantile measured 43x faster at high compression. - LinkedMultiMap this[key]/TryGetValues return NodeValueView in O(1) instead of allocating TValue[]. (Small API change — return type changed from IReadOnlyList<TValue> to NodeValueView; the view still implements IReadOnlyList<TValue> so most consumers compile clean.) - ConcurrentLinkedDictionary.GetEnumerator — snapshot-on-iterate so concurrent writers aren't blocked for the iterator's lifetime. - ObservableList.RemoveAll/RemoveAllAsync — single-pass swap-and-truncate (was O(N*k) descending-index RemoveAt loop). - TimelineArray.ReplayAtFps — O(log N + frames) via linear advance. - BitGrid2D.FillArea / EnumerateSetBits — word-level fast paths. - HyperLogLog.EstimateUnion — no Clone allocation. - TemporalSpatialHashGrid.FindClosestSnapshot — O(log snapshots) via parallel sorted-keys list. The README and class XML doc-comments are now aligned to actual code behavior. Patch releases will continue to honor the PublicApiAnalyzer- baselined surface; this minor bump is for the LinkedMultiMap return-type change and the new GraphDictionary.FindShortestUnweighted method.
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
1 participant
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
Comprehensive complexity audit across all 32 public types, with implementation fixes where the original asymptotic claim was off, plus a doc honesty pass aligning README + XML docs to actual behavior. 15 commits, all green: 1526 net8 / 1497 net6 tests passing, 0 errors.
Backed by a three-stage agent verification process documented in
docs/complexity-audit-2026-05.md(gitignored — local research artifact). Initial pass: 3 reading agents covering all 32 types. Verification pass: 9 independent agents (3 per cluster). Final implementation fixes verified by 3 parallel agents per fix, requiring 3/3 unanimous consensus before commit.Findings actioned
56 of 56 findings from the audit, in priority order:
GraphDictionary.FindNodesWithinDistance— wrong-result bug on weighted graphs (FIFO Queue + weighted relaxation, not BFS or Dijkstra). Replaced with bounded Dijkstra.RebuildFromSorted— drops Merge end-to-end O((c₁+c₂) log) → O(c₁+c₂), Compress O(c log c) → O(c). 2× bench speedup at high c.Breaking change
LinkedMultiMap.this[key]andTryGetValuesreturn type changed fromIReadOnlyList<TValue>toLinkedMultiMap<K,V>.NodeValueView.NodeValueViewimplementsIReadOnlyList<TValue>, so most consumer code continues to compile and run:foreach (var v in mm[key])— worksIReadOnlyList<TValue>— works (interface satisfied)var x = mm[key]; x.Count— works (inferred type changes from interface to concrete; runtime behavior identical)Real breaks for these patterns:
bool TryGetValues(K key, out IReadOnlyList<V> values)—outparameter type must changeTValue[]snapshot immune to subsequent multimap mutations — call.ToArray()for that semanticsThe new contract is: O(1) view construction (no per-call array alloc), aliases the live multimap state, mutates LRU order on read. Previous behavior was O(values per key) array alloc per call, also mutating LRU but undocumented.
Verification methodology
Each implementation change in this PR was verified by 3 parallel agents reading the new code independently. Examples:
Array.Sortper recursion level, average O(N log N).Width[i] of X = cumWeight(X.Forward[i]) - cumWeight(X)holds at every node + level pair; algorithm is single-pass left-to-right with O(c) total work.MoveNext→MoveNext→Disposemid-iteration) and confirmed lock release happens before any yield, writers are unblocked during caller iteration.All implementation changes also have benchmark before/after numbers in their commit messages where applicable. Tier 1 doc-only changes have no perf impact.
Bench evidence (highlights)
KdTree.Fill @ N=100kKdTree.Fill @ N=10k(memory)Digest.Quantile @ C=1000 / N=1kDigest.Merge @ C=1000 / N=1k(All from smoke-profile bench on i7-13700KF; reproduce with
.\bench.ps1 --filter '*<TypeName>Benchmarks*'.)Test status
The 2 skipped are pre-existing (DigestStreamingAnalytics compression-comparison test + BoundedDictionary capacity test, both unrelated to this PR).
Targeting
Base =
dev/omni-collections-v2, mirroring v2.0.0 / v2.0.1 release pattern. Will land for v2.1.0 — minor bump becauseLinkedMultiMapinterface contract still satisfiesIReadOnlyList<TValue>for most consumers (the breaking patterns are narrow).After this merges:
Directory.Build.propsversion 2.0.1 → 2.1.0dev/omni-collections-v2→maingit tag v2.1.0 && git push origin v2.1.0