CDT.NET: Full ReadOnlySpan<T> input migration + round 2 performance optimizations#8
CDT.NET: Full ReadOnlySpan<T> input migration + round 2 performance optimizations#8
Conversation
Convert List<T> arguments to T[] via .ToArray() for ReadOnlySpan<T> parameters, and convert IReadOnlyList<T> (cdt.Triangles) to T[] via .ToArray() as well. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…kups Co-authored-by: MichaCo <5837539+MichaCo@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates CDT.NET’s public and internal APIs away from IReadOnlyList<T> inputs to ReadOnlySpan<T> to reduce virtual/interface dispatch in hot paths, and applies additional allocation and lookup optimizations (notably around fixed-edge queries and peel-layer processing).
Changes:
- Replace
IReadOnlyList<T>inputs withReadOnlySpan<T>across core geometry, utilities, KD-tree, and triangulation APIs. - Add frozen fixed-edge lookup support (
FrozenSet<Edge>) and additional span-based local caching in hot loops. - Update tests to call the new span-based APIs (primarily via
ToArray()).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/CDT.Tests/TriangulationTests.cs | Updates utility calls to pass arrays/spans to match new span-based signatures. |
| test/CDT.Tests/ReadmeExamplesTests.cs | Updates README example tests to call the new span-based triangulation APIs. |
| test/CDT.Tests/GroundTruthTests.cs | Updates ground-truth + KD-tree tests for span-based APIs (introduces repeated ToArray() usage in loops). |
| src/CDT.Core/Types.cs | Changes Box2d<T> helpers to accept ReadOnlySpan<V2d<T>>. |
| src/CDT.Core/Triangulation.cs | Updates triangulation public API to spans; adds frozen fixed-edge lookup and multiple hot-path optimizations. |
| src/CDT.Core/KdTree.cs | Migrates KD-tree APIs to accept ReadOnlySpan<V2d<T>>. |
| src/CDT.Core/CdtUtils.cs | Migrates several utility APIs to spans and adds internal backing fields to DuplicatesInfo for faster internal flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/CDT.Core/Triangulation.cs
Outdated
| /// Freezes the fixed-edge set for faster read-only Contains() queries. | ||
| /// Call after all InsertEdges/ConformToEdges calls are complete. | ||
| /// Invalidated automatically on any subsequent edge mutation. |
There was a problem hiding this comment.
FreezeFixedEdges() claims the frozen set is “invalidated automatically on any subsequent edge mutation”, but there are edge mutations that currently don't clear _frozenFixedEdges (e.g., _fixedEdges is remapped during finalization when removing the super-triangle). Consider ensuring every _fixedEdges mutation path also sets _frozenFixedEdges = null, or narrowing the XML doc to the cases that are actually invalidated.
| /// Freezes the fixed-edge set for faster read-only Contains() queries. | |
| /// Call after all InsertEdges/ConformToEdges calls are complete. | |
| /// Invalidated automatically on any subsequent edge mutation. | |
| /// Freezes (snapshots) the current fixed-edge set for faster read-only Contains() queries. | |
| /// Call this only after all InsertEdges/ConformToEdges operations and any other edge | |
| /// mutations that affect the fixed-edge set are complete, or call it again after such | |
| /// mutations to refresh the frozen snapshot. |
There was a problem hiding this comment.
Fixed in 8c2b7a7. _frozenFixedEdges = null is now set in FinalizeTriangulation when the super-triangle edge keys are remapped (the one mutation path that wasn't covered). The XML doc has been updated to accurately describe that finalization does not auto-refresh the frozen snapshot — callers should call FreezeFixedEdges() again after finalization if needed.
src/CDT.Core/Triangulation.cs
Outdated
| // Frozen set for fast read-only Contains() on fixed edges | ||
| private FrozenSet<Edge>? _frozenFixedEdges; | ||
|
|
||
| // Returns the frozen set when available, falling back to the live HashSet | ||
| private IReadOnlySet<Edge> FixedEdgeSet => _frozenFixedEdges ?? (IReadOnlySet<Edge>)_fixedEdges; | ||
|
|
There was a problem hiding this comment.
FixedEdgeSet is typed as IReadOnlySet<Edge>, so the hot-path Contains() calls become interface dispatch when _frozenFixedEdges is null (the common case unless users call FreezeFixedEdges). Consider branching on _frozenFixedEdges and calling _fixedEdges.Contains(...) / _frozenFixedEdges.Contains(...) directly (or a small helper) to avoid the interface call overhead in IsFlipNeeded, GrowToBoundary, and PeelLayer.
There was a problem hiding this comment.
Fixed in 8c2b7a7. Removed the IReadOnlySet<Edge> FixedEdgeSet property. All three hot-path call sites (IsFlipNeeded, GrowToBoundary, PeelLayer) now capture var frozen = _frozenFixedEdges; once and use frozen != null ? frozen.Contains(e) : _fixedEdges.Contains(e) — direct concrete-type dispatch in both branches with no interface cast overhead.
test/CDT.Tests/GroundTruthTests.cs
Outdated
| for (int i = 0; i < pts.Count; i++) kd.Insert(i, pts.ToArray()); | ||
|
|
||
| // Querying the exact location of each grid point returns that point | ||
| for (int i = 0; i < pts.Count; i++) | ||
| { | ||
| int nearest = kd.Nearest(pts[i].X, pts[i].Y, pts); | ||
| int nearest = kd.Nearest(pts[i].X, pts[i].Y, pts.ToArray()); | ||
| Assert.Equal(i, nearest); |
There was a problem hiding this comment.
These KD-tree tests call pts.ToArray() inside tight loops, which allocates a new array every iteration (O(n) allocations) and can significantly slow the test suite. Materialize the array once (or use CollectionsMarshal.AsSpan(pts) if you want to avoid allocating) and reuse it for all Insert/Nearest calls.
There was a problem hiding this comment.
Fixed in 8c2b7a7. All KD-tree tests now use CollectionsMarshal.AsSpan(pts) materialized once before the loops — no per-iteration heap allocations. The stress test especially benefits since it previously allocated 100 + 1000 arrays.
|
@copilot fix viz app build errors |
…FixedEdgeSet dispatch Co-authored-by: MichaCo <5837539+MichaCo@users.noreply.github.com>
…pan conversions Co-authored-by: MichaCo <5837539+MichaCo@users.noreply.github.com>
Fixed in 150f3fb. Added |
|
@copilot Fix the benchmarks, make BenchmarkInputReader.Read return 2 arrays |
… field types Co-authored-by: MichaCo <5837539+MichaCo@users.noreply.github.com>
Fixed in 6fc4132. |
|
This literarily has no effect on performance or memory, at all. |
ReadOnlySpan<T>input migration across Types.cs, CdtUtils.cs, KdTree.cs, Triangulation.cs.ToArray()/CollectionsMarshal.AsSpanfor span conversionsCollectionsMarshal.AsSpan(pts)once before loopsFreezeFixedEdgesdoc/invalidation + removeIReadOnlySet<Edge>interface dispatchCollectionsMarshal.AsSpanforList<T>→ span at call sitesBenchmarkInputReader.Readnow returns(V2d<double>[], Edge[])instead of(List<V2d<double>>, List<Edge>); field types in both benchmark classes changed to arrays;FloatVsDouble_FloatusesArray.ConvertAllinstead of LINQ.Select().ToList()✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.