And linq #895
And linq #895
Conversation
This is a partial set of clean-ups. There could be others, but they may be controversial (e.g. adopting LINQ in some areas, which may or may not hurt performance). I've also tried to be mindful of target framework versions and also some edge cases (e.g. hashing) where seemingly redundant chunks of code, if removed or changed for another option, may blow things up. See what you think.
Pre-change: // * Summary * BenchmarkDotNet=v0.12.1, OS=endeavouros AMD Ryzen 7 5800H with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores .NET Core SDK=8.0.108 [Host] : .NET Core 8.0.8 (CoreCLR 8.0.824.36612, CoreFX 8.0.824.36612), X64 RyuJIT Job-GRIUMJ : .NET Core 8.0.8 (CoreCLR 8.0.824.36612, CoreFX 8.0.824.36612), X64 RyuJIT IterationCount=1 LaunchCount=1 WarmupCount=1 | Method | EdgeCount | Mean | Error | Gen 0 | Gen 1 | Gen 2 | Allocated | |--------------- |---------- |-----------:|------:|----------:|----------:|----------:|----------:| | Intersection_N | 1000 | 119.9 ms | NA | 1000.0000 | - | - | 9.04 MB | | Intersection_N | 2000 | 542.1 ms | NA | 1000.0000 | - | - | 16.2 MB | | Intersection_N | 3000 | 1,431.1 ms | NA | 5000.0000 | 4000.0000 | 1000.0000 | 37.28 MB | | Intersection_N | 4000 | 3,008.9 ms | NA | 6000.0000 | 5000.0000 | 1000.0000 | 47.41 MB | | Intersection_N | 5000 | 6,173.7 ms | NA | 9000.0000 | 8000.0000 | 1000.0000 | 74.05 MB | // * Legends * EdgeCount : Value of the 'EdgeCount' parameter Mean : Arithmetic mean of all measurements Error : Half of 99.9% confidence interval Gen 0 : GC Generation 0 collects per 1000 operations Gen 1 : GC Generation 1 collects per 1000 operations Gen 2 : GC Generation 2 collects per 1000 operations Allocated : Allocated memory per single operation (managed only, inclusive, 1KB = 1024B) 1 ms : 1 Millisecond (0.001 sec) // * Diagnostic Output - MemoryDiagnoser * // ***** BenchmarkRunner: End ***** // ** Remained 0 benchmark(s) to run ** Run time: 00:00:47 (47.83 sec), executed benchmarks: 5 Global total time: 00:00:57 (57.28 sec), executed benchmarks: 5 Post-change: // * Summary * BenchmarkDotNet=v0.12.1, OS=endeavouros AMD Ryzen 7 5800H with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores .NET Core SDK=8.0.108 [Host] : .NET Core 8.0.8 (CoreCLR 8.0.824.36612, CoreFX 8.0.824.36612), X64 RyuJIT Job-WWTCPL : .NET Core 8.0.8 (CoreCLR 8.0.824.36612, CoreFX 8.0.824.36612), X64 RyuJIT IterationCount=1 LaunchCount=1 WarmupCount=1 | Method | EdgeCount | Mean | Error | Gen 0 | Gen 1 | Gen 2 | Allocated | |--------------- |---------- |-----------:|------:|----------:|----------:|----------:|----------:| | Intersection_N | 1000 | 127.1 ms | NA | - | - | - | 6.85 MB | | Intersection_N | 2000 | 475.1 ms | NA | 2000.0000 | 1000.0000 | - | 23.04 MB | | Intersection_N | 3000 | 1,408.1 ms | NA | 5000.0000 | 4000.0000 | 1000.0000 | 36.15 MB | | Intersection_N | 4000 | 2,874.6 ms | NA | 5000.0000 | 4000.0000 | 1000.0000 | 41.76 MB | | Intersection_N | 5000 | 5,954.3 ms | NA | 9000.0000 | 8000.0000 | 2000.0000 | 68.62 MB | // * Legends * EdgeCount : Value of the 'EdgeCount' parameter Mean : Arithmetic mean of all measurements Error : Half of 99.9% confidence interval Gen 0 : GC Generation 0 collects per 1000 operations Gen 1 : GC Generation 1 collects per 1000 operations Gen 2 : GC Generation 2 collects per 1000 operations Allocated : Allocated memory per single operation (managed only, inclusive, 1KB = 1024B) 1 ms : 1 Millisecond (0.001 sec) // * Diagnostic Output - MemoryDiagnoser * // ***** BenchmarkRunner: End ***** // ** Remained 0 benchmark(s) to run ** Run time: 00:00:46 (46.34 sec), executed benchmarks: 5 Global total time: 00:00:51 (51.28 sec), executed benchmarks: 5
|
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. |
|
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 |
|
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. |
| if (pt.y < bounds.top) bounds.top = pt.y; | ||
| if (pt.y > bounds.bottom) bounds.bottom = pt.y; | ||
| } | ||
| foreach (PointD pt in from pi in PolyInfoList from path in pi.paths from pt in path select pt) |
There was a problem hiding this comment.
This is much less easy to debug now.
|
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. |
|
Thanks for the feedback JBSP. I have a low threshold to reverting the linq dependency. |
| @@ -382,12 +366,11 @@ public static Path64 ScalePath(Path64 path, double scale) | |||
| { | |||
| if (InternalClipper.IsAlmostZero(scale - 1)) return path; | |||
| Path64 result = new Path64(path.Count); | |||
| result.AddRange(path.Select(pt => new Point64(pt.X * scale, pt.Y * scale))); | |||
There was a problem hiding this comment.
This doesn't look right; The statement was moved outside of the !USINGZ block but the case for USINGZ is still there.
|
I've now reversed the LINQ dependency. |
|
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. |
|
HI Phil. I really appreciate your efforts to improve the library. |
|
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. |
Yep, I'd be happy to add more CI testing (including C#). |
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.