Conversation
stefannikolei
commented
Jul 6, 2025
stefannikolei
commented
- Added an initial Capacity which is estimated through the amount of segments
- Used unsafe in The PriorityQueue to get a speed increase
* Added an initial Capacity which is estimated through the amount of segments * Used unsafe in The PriorityQueue to get a speed increase
There was a problem hiding this comment.
Pull Request Overview
Optimizes the StablePriorityQueue by switching from a List to an array-backed heap with unsafe inlined operations and introduces an initial capacity estimate in PolygonClipper.
- Replaced List with a T[] buffer, manual
counttracking, and unsafeMemoryMarshal/Unsafe.Addfor performance. - Added a constructor overload to specify initial capacity and implemented an automatic
Resizemethod. - Updated PolygonClipper to estimate and pass an initial capacity to the priority queue based on polygon segment counts.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/PolygonClipper/StablePriorityQueue{T,TComparer}.cs | Swapped out List<T> for a raw array, added capacity-based ctor, inlined Enqueue, Dequeue, Up, Down, and Resize. |
| src/PolygonClipper/PolygonClipper.cs | Replaced the default queue instantiation with one that takes an estimatedEventCount computed from segment counts. |
Comments suppressed due to low confidence (2)
src/PolygonClipper/StablePriorityQueue{T,TComparer}.cs:90
- The
Peekmethod was removed, which breaks existing API by eliminating non-destructive access to the top element. Consider restoringPeekor providing equivalent functionality.
else
src/PolygonClipper/StablePriorityQueue{T,TComparer}.cs:29
- Add tests to verify that the initial capacity is correctly applied and that the queue resizes properly when the number of enqueued items exceeds the provided capacity.
public StablePriorityQueue(TComparer comparer, int capacity = 4)
| Vertex max = new(double.NegativeInfinity); | ||
| StablePriorityQueue<SweepEvent, SweepEventComparer> eventQueue = new(new SweepEventComparer()); | ||
|
|
||
| int subjectSegments = subject.ContourCount > 0 ? subject[0].VertexCount - 1 : 0; |
There was a problem hiding this comment.
The estimated segment count only accounts for the first contour via subject[0]. If multiple contours exist, this underestimates required capacity. Consider summing segments across all contours instead.
| int subjectSegments = subject.ContourCount > 0 ? subject[0].VertexCount - 1 : 0; | |
| int subjectSegments = 0; | |
| for (int i = 0; i < subject.ContourCount; i++) | |
| { | |
| subjectSegments += subject[i].VertexCount - 1; | |
| } |
There was a problem hiding this comment.
The bot has a valid point here.
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
I'm not sure about this PR. I think we need to do some work here for correctness and safety along with performance.
| Vertex max = new(double.NegativeInfinity); | ||
| StablePriorityQueue<SweepEvent, SweepEventComparer> eventQueue = new(new SweepEventComparer()); | ||
|
|
||
| int subjectSegments = subject.ContourCount > 0 ? subject[0].VertexCount - 1 : 0; |
There was a problem hiding this comment.
The bot has a valid point here.
| if (this.heap.Count == 0) | ||
| if (this.count == 0) | ||
| { | ||
| throw new InvalidOperationException("Queue is empty."); |
There was a problem hiding this comment.
This should be in a non-inlining throwhelper
| heapRef = Unsafe.Add(ref heapRef, this.count); | ||
|
|
||
| // Clear the last position to avoid holding references | ||
| Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(this.heap), this.count) = default(T)!; |
There was a problem hiding this comment.
I don't believe the behavior is the same as the original code here and we are doing stuff (assigning default(T)!) when we shouldn't (structs).
| /// </summary> | ||
| /// <returns>The item with the highest priority.</returns> | ||
| /// <exception cref="InvalidOperationException">Thrown if the priority queue is empty.</exception> | ||
| public T Peek() |
There was a problem hiding this comment.
I would have kept this purely to complete the implementation.
| List<T> data = this.heap; | ||
| T item = data[index]; | ||
| ref T heapRef = ref MemoryMarshal.GetArrayDataReference(this.heap); | ||
| T item = Unsafe.Add(ref heapRef, index); |
There was a problem hiding this comment.
Here and elsewhere Unsafe.Add produces poor codegen for int. You should use uint or nuint
| /// Resizes the internal array when capacity is exceeded. | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| private void Resize() |
There was a problem hiding this comment.
We should be resizing using the same rules as List<T>
https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs,488
Would you think this is a feasible way. I improved the performance. Just forgot to post the benchmark. Or do you see an alternative to using unsafe? (Unsafe is a new land for me😅) |
|
I think we can and should use unsafe we just have to be very careful when doing so. |
