Skip to content
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.

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

Improve list optimisation in CountDown #942

Merged
merged 2 commits into from
Jan 24, 2023

Conversation

atifaziz
Copy link
Member

@atifaziz atifaziz commented Jan 23, 2023

CountDown has a path optimised for when the source parameter is a list, but it was poorly written respect to performance. It accessed the list Count property 3 times, twice during each iteration of the loop. Each access is a virtual dispatch so it seems to adds up, making it perform worse than when the source is a sequence (zero net optimisation). This PR fixes that by accessing Count once, storing it in a local and then using the local throughout. This improves the performance by 40%.

Before this PR

Method Source Mean Error StdDev Median Allocated
MoreLinqCountDown Int32[1000000] 21.31 ms 0.426 ms 1.209 ms 21.02 ms 119 B
MoreLinqCountDown Syste(...)rator [36] 19.30 ms 0.496 ms 1.440 ms 18.54 ms 179 B

See benchmarks-before.log.txt for full details.

After this PR

Method Source Mean Error StdDev Median Allocated
MoreLinqCountDown Int32[1000000] 12.83 ms 0.318 ms 0.934 ms 12.63 ms 100 B
MoreLinqCountDown Syste(...)rator [36] 20.65 ms 0.631 ms 1.851 ms 20.01 ms 179 B

See benchmarks-after.log.txt for full details.

Additional Information

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22621.1105)
Intel Core i7-1065G7 CPU 1.30GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.102
  [Host] : .NET 7.0.2 (7.0.222.60605), X86 RyuJIT AVX2

See also:

@atifaziz atifaziz added this to the 3.4.0 milestone Jan 23, 2023
@atifaziz atifaziz self-assigned this Jan 23, 2023
MoreLinq/CountDown.cs Outdated Show resolved Hide resolved
MoreLinq/CountDown.cs Outdated Show resolved Hide resolved
@viceroypenguin
Copy link
Contributor

Two things:

  1. I would rerun the performance benchmark after making the changes to fix the bugs above. The missed count value could have a performance impact on the behavior.
  2. If possible, I would copy the before/after into LinqPAD for benchmarking. The reason is because two separate benchmarks, even on the same machine, are not always comparable (other processes, etc.). It is more accurate to have a Before() method and an After() method in LinqPAD and benchmark them against each other in the same run.

@atifaziz
Copy link
Member Author

atifaziz commented Jan 23, 2023

  1. I would rerun the performance benchmark after making the changes to fix the bugs above. The missed count value could have a performance impact on the behavior.

45% improvement:

Method Source Mean Error StdDev Median Allocated
MoreLinqCountDown Int32[1000000] 11.27 ms 0.316 ms 0.923 ms 11.02 ms 100 B
MoreLinqCountDown Syste(...)rator [36] 20.61 ms 0.707 ms 2.041 ms 20.58 ms 179 B
  1. If possible, I would copy the before/after into LinqPAD for benchmarking. The reason is because two separate benchmarks, even on the same machine, are not always comparable (other processes, etc.). It is more accurate to have a Before() method and an After() method in LinqPAD and benchmark them against each other in the same run.

Yes, I am aware and admit taking a shortcut there. The difference was large enough and comparable in the many runs I did. What's more important is the comparison between list and sequence processing in the same ran than between the before and after

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #942 (da13c3f) into master (d587f73) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #942      +/-   ##
==========================================
- Coverage   92.42%   92.41%   -0.01%     
==========================================
  Files         112      112              
  Lines        3446     3442       -4     
  Branches     1023     1023              
==========================================
- Hits         3185     3181       -4     
  Misses        199      199              
  Partials       62       62              
Impacted Files Coverage Δ
MoreLinq/CountDown.cs 90.00% <100.00%> (-1.18%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@atifaziz atifaziz merged commit 9c17aae into morelinq:master Jan 24, 2023
@atifaziz atifaziz deleted the faster-count-down-list-loop branch January 24, 2023 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants