AngusJohnson / Clipper2

Polygon Clipping and Offsetting - C++, C# and Delphi
Boost Software License 1.0
1.52k stars 273 forks source link

And linq #895

Closed philstopford closed 1 month ago

philstopford commented 1 month ago

With LINQ on top, benchmark doesn't show any performance regression (and any win may also be purely based on the circumstances). It looks like a wash overall, but it makes the code a little cleaner to my eye and that could be a good thing.

AngusJohnson commented 1 month ago

Are there any downsides to using LINQ (apart from the potential to impact performance that your tests have pretty much discounted)? If there aren't, I'd be happy to merge.

philstopford commented 1 month ago

No real disadvantages that I can see. It's not an automatic win/loss on performance, hence the test data being useful in making a suggestion about this change. I also saw no measurable impact from these changes overall in my end-user applications based around Clipper.

In the case of this library, the LINQ candidates are pretty straightforward to understand. I admit that I often decline LINQ single-liners that others would go for, just because they are, to me, not easily readable, but here I had no concerns.

LINQ can also be a little tricky to debug, if you use it like a hammer, but here, I don't really see a concern, based on how and where it is used. Errors can be raised and linked back to a different part of the code and that needs to be born in mind.

There's a fairly decent summary here : https://stackoverflow.com/questions/271384/pros-and-cons-of-linq-language-integrated-query

AngusJohnson commented 1 month ago

Thanks Phil. I really appreciate what you've done.

Edit: I note that there's a conflict that's stopping the merge. Are you able to resolve this?

philstopford commented 1 month ago

Thanks Phil. I really appreciate what you've done.

Edit: I note that there's a conflict that's stopping the merge. Are you able to resolve this?

Looks trivial - I'll sort it out on my end (collision between the clean-up merge and this PR) and update shortly. Stay tuned.

JimBobSquarePants commented 1 month ago

I don't mean to rain on anyones parade here, but I would introduce LINQ into high performance scenarios as an absolute last resort. There are often hidden allocations that are hard to spot plus debugging of stements becomes less trivial in complex sections (see highlighted comment).

While no change was seen in the current benchmarks there's no guarantee that the benchmarks themseleves provide adequate coverage.

AngusJohnson commented 1 month ago

Thanks for the feedback JBSP. I have a low threshold to reverting the linq dependency.

AngusJohnson commented 1 month ago

I've now reversed the LINQ dependency.

philstopford commented 1 month ago

I ran a suite of tests using applications that make heavy use of Clipper and saw no performance impact. I expect I also had some clean-up code in my use cases that stripped duplicate vertices. It might be wise to have test coverage for the duplicates, though, to avoid similar 'regressions' in future - reverting the PR is fine, but the door is still wide open to a similar regression in future.

AngusJohnson commented 1 month ago

HI Phil. I really appreciate your efforts to improve the library. I had a low threshold to reverse your linq dependency because I personally found it harder to read the code. But my knowledge of C# is extremely limited, so I'm very dependant on feedback from the library's C# users. I'm sorry if I reverted some cleanup code too. I thought I'd only reverted the linq specific changes. Anyhow, I'm not sure what 'duplicates' or 'regressions' you're referring to.

philstopford commented 1 month ago

No worries - LINQ is always something of a love/hate/indifference thing, and people tend to avoid it also on the assumption it will mess up performance (which I could find no case where it impacted performance in my tests). My comment about the regression was for the now-reverted change in line 369. I use USINGZ quite a bit and it never flagged on my side and perhaps there needs to be some test coverage to avoid a similar mistake in future. Failing the test would have avoided the oversight, I suspect.

AngusJohnson commented 1 month ago

I use USINGZ quite a bit and it never flagged on my side and perhaps there needs to be some test coverage to avoid a similar mistake in future.

Yep, I'd be happy to add more CI testing (including C#). But I'm not overly motivated to doing this myself, especially for C#, since my focus is primarily on ensuring the C++ code is robust (in every way).