Skip to content

CDT.NET: Full ReadOnlySpan<T> input migration + round 2 performance optimizations#8

Closed
Copilot wants to merge 6 commits intomainfrom
copilot/replace-ireadonlylist-with-readonlyspan
Closed

CDT.NET: Full ReadOnlySpan<T> input migration + round 2 performance optimizations#8
Copilot wants to merge 6 commits intomainfrom
copilot/replace-ireadonlylist-with-readonlyspan

Conversation

Copy link
Contributor

Copilot AI commented Feb 22, 2026

  • Part 1a-1e: Full ReadOnlySpan<T> input migration across Types.cs, CdtUtils.cs, KdTree.cs, Triangulation.cs
  • Part 2A-2D: Performance optimizations (span locals, FrozenSet, stack reuse, PeelLayer allocation elimination)
  • Update test files to use .ToArray() / CollectionsMarshal.AsSpan for span conversions
  • Fix KD-tree tests: materialize CollectionsMarshal.AsSpan(pts) once before loops
  • Fix FreezeFixedEdges doc/invalidation + remove IReadOnlySet<Edge> interface dispatch
  • Fix viz app build errors: CollectionsMarshal.AsSpan for List<T> → span at call sites
  • Fix benchmark build errors: BenchmarkInputReader.Read now returns (V2d<double>[], Edge[]) instead of (List<V2d<double>>, List<Edge>); field types in both benchmark classes changed to arrays; FloatVsDouble_Float uses Array.ConvertAll instead of LINQ .Select().ToList()
  • All 997 tests passing
  • CDT.Viz and CDT.Benchmarks build with 0 errors

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits February 22, 2026 19:21
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>
Copilot AI changed the title [WIP] Migrate IReadOnlyList<T> to ReadOnlySpan<T> for performance CDT.NET: Full ReadOnlySpan<T> input migration + round 2 performance optimizations Feb 22, 2026
Copilot AI requested a review from MichaConrad February 22, 2026 19:27
@MichaConrad MichaConrad requested a review from Copilot February 22, 2026 19:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with ReadOnlySpan<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.

Comment on lines +322 to +324
/// 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.
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
/// 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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +115 to +120
// 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;

Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 715 to 721
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);
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot apply changes based on this feedback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MichaConrad
Copy link
Collaborator

@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>
Copy link
Contributor Author

Copilot AI commented Feb 22, 2026

@copilot fix viz app build errors

Fixed in 150f3fb. Added using System.Runtime.InteropServices and updated the three call sites in MainWindow.xaml.cs to use CollectionsMarshal.AsSpan(workPts) and CollectionsMarshal.AsSpan(workEdges) for InsertVertices, InsertEdges, and ConformToEdges. CDT.Viz now builds with 0 errors.

@MichaConrad
Copy link
Collaborator

@copilot Fix the benchmarks, make BenchmarkInputReader.Read return 2 arrays

… field types

Co-authored-by: MichaCo <5837539+MichaCo@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 22, 2026

@copilot Fix the benchmarks, make BenchmarkInputReader.Read return 2 arrays

Fixed in 6fc4132. BenchmarkInputReader.Read now returns (V2d<double>[] Vertices, Edge[] Edges) — arrays allocate exactly once and implicitly convert to ReadOnlySpan<T> at every call site with no overhead. Both benchmark class fields updated to V2d<double>[] / Edge[], and FloatVsDouble_Float uses Array.ConvertAll instead of LINQ. CDT.Benchmarks builds with 0 errors.

@MichaConrad
Copy link
Collaborator

This literarily has no effect on performance or memory, at all.

@MichaConrad MichaConrad deleted the copilot/replace-ireadonlylist-with-readonlyspan branch February 22, 2026 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants