Reduce Allocations by removing creation of List<SweepEvent> = new(4)#12
Reduce Allocations by removing creation of List<SweepEvent> = new(4)#12stefannikolei wants to merge 3 commits intoSixLabors:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the allocation of a temporary List<SweepEvent> in PossibleIntersection and replaces it with four nullable local variables plus a flag, adding null checks before use.
- Replaced
List<SweepEvent> eventswithSweepEvent? first/second/third/fourthandbool firstSet - Updated the logic for assigning and ordering these variables instead of
events.Add - Inserted
ArgumentNullException.ThrowIfNullcalls before eachDivideSegmentinvocation
Comments suppressed due to low confidence (1)
src/PolygonClipper/PolygonClipper.cs:572
- [nitpick] Variable names
first,second,third, andfourthare ambiguous. Consider renaming them to indicate their roles (e.g.,startEventA,startEventB,endEventA,endEventB) for better clarity.
SweepEvent? first = null;
| ? (le2.OtherEvent, le1.OtherEvent) | ||
| : (le1.OtherEvent, le2.OtherEvent); | ||
|
|
||
| if (!firstSet) |
There was a problem hiding this comment.
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.
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
Ignore the bot and see my comment regarding passing a span to the method.
| // The line segments associated with le1 and le2 overlap. | ||
| // TODO: Rewrite this to avoid allocation. | ||
| List<SweepEvent> events = new(4); | ||
| SweepEvent? first = null; |
There was a problem hiding this comment.
I would attack this a different way by creating a span outside of the loop and passing it to the private method as a parameter.
There was a problem hiding this comment.
Will look into that.
| { | ||
| DivideSegment(events[0], events[1].Point, eventQueue, comparer); | ||
| DivideSegment(events[1], events[2].Point, eventQueue, comparer); | ||
| ArgumentNullException.ThrowIfNull(first); |
There was a problem hiding this comment.
All these guard checks would kill performance as we're calling this method from a while loop.
There was a problem hiding this comment.
Good to know. That was the NRE brain doing its work 😊
|
BaseLine:
NoSpan:
WithSpan:
|
| ? (le2.OtherEvent, le1.OtherEvent) | ||
| : (le1.OtherEvent, le2.OtherEvent); | ||
|
|
||
| if (!firstSet) |
There was a problem hiding this comment.
I think we've still got bits of the two different optimization attempts here.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I’m having a crack just now. I think I have it worked out in a neat way.
This eliminates the allocation of a new List in the method PossibleIntersection