Optimize PossibleIntersection and StablePriorityQueue#16
Optimize PossibleIntersection and StablePriorityQueue#16JimBobSquarePants merged 3 commits intomainfrom
Conversation
|
LGTM. But whoever designed that Unsafe API... .Add to retrieve a value 🙈🙈 |
Haha yeah, it can be clunky! I just pushed an update for the StablePriorityQueue also. This takes a few percent off the time. |
There was a problem hiding this comment.
Pull Request Overview
This PR applies performance optimizations by replacing temporary allocations in the sweep‐line intersection logic and the priority queue with a reusable buffer and unsafe memory access, and updates supporting tests and benchmarks to use the new APIs.
- Introduce a reusable
Span<SweepEvent>workspace inPossibleIntersectionand switch from list allocations toUnsafe/MemoryMarshaloperations. - Add a capacity-aware constructor to
StablePriorityQueueand refactor itsUp/Downmethods to useCollectionsMarshalandUnsafe.Addfor faster heap adjustments. - Update tests, utilities, and benchmarks to call the new overloads and utilize C# 12 collection expressions.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/PolygonClipper.Tests/TestPolygonUtilitiesTests.cs | New unit tests for ConvertToPolygon and union operations |
| tests/PolygonClipper.Tests/TestPolygonUtilities.cs | Conversion helpers updated to C# 12 collection expressions |
| tests/PolygonClipper.Tests/TestData.cs | Added null-forgiving operators on deserialization |
| tests/PolygonClipper.Tests/PolygonClipper.Tests.csproj | Added Clipper2 package reference for tests |
| tests/PolygonClipper.Tests/GenericTestCases.cs | Replaced .ToList() with a collection expression for features |
| tests/PolygonClipper.Benchmarks/PolygonClipper.Benchmarks.csproj | Linked updated test utilities and bumped Clipper2 version |
| tests/PolygonClipper.Benchmarks/ClippingLibraryComparison.cs | Benchmarks updated to use TestPolygonUtilities and renamed method |
| src/PolygonClipper/StablePriorityQueue{T,TComparer}.cs | Added capacity constructor; refactored Up/Down to use unsafe spans |
| src/PolygonClipper/PolygonClipper.cs | Added workspace span to PossibleIntersection and updated calls |
Comments suppressed due to low confidence (1)
tests/PolygonClipper.Tests/TestPolygonUtilities.cs:89
- This comment appears in
ConvertToClipper2Polygonbut the method returns a Clipper2PathsD, not your internalPolygontype. Update the comment to reflect the actual return type, for example:// Convert GeoJSON Polygon to Clipper2 PathsD.
// Convert GeoJSON Polygon to our Polygon type
| /// </summary> | ||
| /// <param name="index">The index of the item to move upward.</param> | ||
| private void Up(int index) | ||
| /// <param name="index">The index of the newly added item to sift upward.</param> |
There was a problem hiding this comment.
Typo sift --> shift
There was a problem hiding this comment.
Actually, that's intentional, sift is standard terminology in heap operations
A rework of #12 and #13 with added unsafe code!
Note: I've discovered that Clipper2 does not return the correct result for the test data so pay attention to our own results only.
CC @stefannikolei
Main
PR (PossibleIntersection)
PR (StablePriorityQueue)