Skip to content

Reduce Allocations by removing creation of List<SweepEvent> = new(4) #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stefannikolei wants to merge 3 commits into from
Closed

Reduce Allocations by removing creation of List<SweepEvent> = new(4) #12

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2f83fd
Reduce Allocations by removing creation of List<SweepEvent> = new(4)
stefannikolei Jul 6, 2025
cdaa25d
Use Span
stefannikolei Jul 12, 2025
e277131
Merge branch 'main' into sn/performance
JimBobSquarePants Jul 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 37 additions & 22 deletions src/PolygonClipper/PolygonClipper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@

SweepEvent? prevEvent;
SweepEvent? nextEvent;
Span<SweepEvent> lineSegmentsList = new SweepEvent[4].AsSpan();
while (eventQueue.Count > 0)
{
SweepEvent sweepEvent = eventQueue.Dequeue();
Expand All @@ -188,7 +189,7 @@
if (nextEvent != null)
{
// Check intersection with the next neighbor
if (PossibleIntersection(sweepEvent, nextEvent, eventQueue) == 2)
if (PossibleIntersection(sweepEvent, nextEvent, eventQueue, lineSegmentsList) == 2)
{
ComputeFields(sweepEvent, prevEvent, operation);
ComputeFields(nextEvent, sweepEvent, operation);
Expand All @@ -199,7 +200,7 @@
if (prevEvent != null)
{
// Check intersection with the previous neighbor
if (PossibleIntersection(prevEvent, sweepEvent, eventQueue) == 2)
if (PossibleIntersection(prevEvent, sweepEvent, eventQueue, lineSegmentsList) == 2)
{
SweepEvent? prevPrevEvent = statusLine.Prev(prevEvent.PosSL);
ComputeFields(prevEvent, prevPrevEvent, operation);
Expand All @@ -218,7 +219,7 @@
// Check intersection between neighbors
if (prevEvent != null && nextEvent != null)
{
_ = PossibleIntersection(prevEvent, nextEvent, eventQueue);
_ = PossibleIntersection(prevEvent, nextEvent, eventQueue, lineSegmentsList);
}

statusLine.RemoveAt(it);
Expand Down Expand Up @@ -499,6 +500,7 @@
/// <param name="le1">The first sweep event representing a line segment.</param>
/// <param name="le2">The second sweep event representing a line segment.</param>
/// <param name="eventQueue">The event queue to add new events to.</param>
/// <param name="lineSegmentsList">A Span which will be used to store the associations between the line segments</param>
/// <returns>
/// An integer indicating the result of the intersection:
/// <list type="bullet">
Expand All @@ -514,7 +516,8 @@
private static int PossibleIntersection(
SweepEvent le1,
SweepEvent le2,
StablePriorityQueue<SweepEvent, SweepEventComparer> eventQueue)
StablePriorityQueue<SweepEvent, SweepEventComparer> eventQueue,
Span<SweepEvent> lineSegmentsList)
{
if (le1.OtherEvent == null || le2.OtherEvent == null)
{
Expand Down Expand Up @@ -569,8 +572,12 @@
}

// The line segments associated with le1 and le2 overlap.
// TODO: Rewrite this to avoid allocation.
List<SweepEvent> events = new(4);
// lineSegmentsList[0] = null;
// lineSegmentsList[1] = null;
// lineSegmentsList[2] = null;
// lineSegmentsList[3] = null;
bool firstSet = false;

bool leftCoincide = le1.Point == le2.Point;
bool rightCoincide = le1.OtherEvent.Point == le2.OtherEvent.Point;

Expand All @@ -579,27 +586,33 @@
{
if (comparer.Compare(le1, le2) > 0)
{
events.Add(le2);
events.Add(le1);
lineSegmentsList[0] = le2;
lineSegmentsList[1] = le1;
}
else
{
events.Add(le1);
events.Add(le2);
lineSegmentsList[0] = le1;
lineSegmentsList[1] = le2;
}

firstSet = true;
}

if (!rightCoincide)
{
if (comparer.Compare(le1.OtherEvent, le2.OtherEvent) > 0)
(SweepEvent? rightFirst, SweepEvent? rightSecond) = comparer.Compare(le1.OtherEvent, le2.OtherEvent) > 0
? (le2.OtherEvent, le1.OtherEvent)
: (le1.OtherEvent, le2.OtherEvent);

if (!firstSet)
Copy link

Copilot AI Jul 6, 2025

Choose a reason for hiding this comment

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

The firstSet flag is never updated in this branch when assigning first and second, which can lead to third and fourth remaining null incorrectly. Add firstSet = true; after second = rightSecond; to maintain the intended flow.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@JimBobSquarePants JimBobSquarePants Jul 14, 2025

Choose a reason for hiding this comment

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

I think we've still got bits of the two different optimization attempts here.

Copy link
Copy Markdown
Contributor Author

@stefannikolei stefannikolei Jul 15, 2025

Choose a reason for hiding this comment

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

With list .add was used. Now it is added with the index. So I need a way to track that. You have another idea how to do it

Copy link
Copy Markdown
Member

@JimBobSquarePants JimBobSquarePants Jul 15, 2025

Choose a reason for hiding this comment

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

I’m having a crack just now. I think I have it worked out in a neat way.

{
events.Add(le2.OtherEvent);
events.Add(le1.OtherEvent);
lineSegmentsList[0] = rightFirst;
lineSegmentsList[1] = rightSecond;
}
else
{
events.Add(le1.OtherEvent);
events.Add(le2.OtherEvent);
lineSegmentsList[2] = rightFirst;
lineSegmentsList[3] = rightSecond;
}
}

Expand All @@ -612,8 +625,9 @@
: EdgeType.DifferentTransition;

if (leftCoincide && !rightCoincide)
{

Check warning on line 628 in src/PolygonClipper/PolygonClipper.cs

View workflow job for this annotation

GitHub Actions / Build (ubuntu-latest, net9.0, 9.0.x, true, -x64, false)

Check warning on line 628 in src/PolygonClipper/PolygonClipper.cs

View workflow job for this annotation

GitHub Actions / Build (windows-latest, net9.0, 9.0.x, true, -x64, false)

Check warning on line 628 in src/PolygonClipper/PolygonClipper.cs

View workflow job for this annotation

GitHub Actions / Build (windows-latest, net8.0, 8.0.x, -x64, false)

DivideSegment(events[1].OtherEvent, events[0].Point, eventQueue, comparer);

DivideSegment(lineSegmentsList[1].OtherEvent, lineSegmentsList[0].Point, eventQueue, comparer);
}

return 2;
Expand All @@ -622,21 +636,22 @@
// Handle the rightCoincide case
if (rightCoincide)
{
DivideSegment(events[0], events[1].Point, eventQueue, comparer);
DivideSegment(lineSegmentsList[0], lineSegmentsList[1].Point, eventQueue, comparer);
return 3;
}

// Handle general overlapping case
if (events[0] != events[3].OtherEvent)
if (lineSegmentsList[0] != lineSegmentsList[3].OtherEvent)
{

Check warning on line 645 in src/PolygonClipper/PolygonClipper.cs

View workflow job for this annotation

GitHub Actions / Build (ubuntu-latest, net9.0, 9.0.x, true, -x64, false)

Check warning on line 645 in src/PolygonClipper/PolygonClipper.cs

View workflow job for this annotation

GitHub Actions / Build (windows-latest, net9.0, 9.0.x, true, -x64, false)

Check warning on line 645 in src/PolygonClipper/PolygonClipper.cs

View workflow job for this annotation

GitHub Actions / Build (windows-latest, net8.0, 8.0.x, -x64, false)

DivideSegment(events[0], events[1].Point, eventQueue, comparer);
DivideSegment(events[1], events[2].Point, eventQueue, comparer);

DivideSegment(lineSegmentsList[0], lineSegmentsList[1].Point, eventQueue, comparer);
DivideSegment(lineSegmentsList[1], lineSegmentsList[2].Point, eventQueue, comparer);
return 3;
}

// One segment fully contains the other
DivideSegment(events[0], events[1].Point, eventQueue, comparer);
DivideSegment(events[3].OtherEvent, events[2].Point, eventQueue, comparer);
DivideSegment(lineSegmentsList[0], lineSegmentsList[1].Point, eventQueue, comparer);
DivideSegment(lineSegmentsList[3].OtherEvent, lineSegmentsList[2].Point, eventQueue, comparer);
return 3;
}

Expand Down
Loading