Closed naricc closed 3 years ago
Tagging subscribers to this area: @brzvlad See info in area-owners.md if you want to be subscribed.
Author: | naricc |
---|---|
Assignees: | - |
Labels: | `area-Codegen-Interpreter-mono`, `tenet-performance` |
Milestone: | - |
@naricc, was there a corresponding 50% improvement in the same benchmarks after https://github.com/dotnet/runtime/pull/44265 went in?
No there was not. Here is the graph
You can see that we got a win, but then after we reverted the change we are worse than we were before.
It's likely 8e6415dd1c0efc112cbd93cc58b3e87bc2b4ad1c that caused the ConcurrentQueue
regressions, that was expected. Based on what I saw before, the test is not very realistic in measuring contended performance and any spin-waiting in ConcurrentQueue.Enqueue/Dequeue
would have the effect of turning this benchmark into a more single-threaded benchmark and its perf benefits from that. The large gain in the microbenchmark from https://github.com/dotnet/runtime/pull/44265 also coincided with most of the large regressions in https://github.com/dotnet/runtime/issues/45716. From the perf data in https://github.com/dotnet/runtime/pull/46120 it seems clear that the change works well, even though it regresses this perf test.
Not sure about the other tests, will have to see.
I filed an issue with all the tests that we are seeing with this issue, it is here https://github.com/dotnet/runtime/issues/46714. Mostly they regressed tests are in Concurrent* classes and we had a few in RentReturnArrayPool tests.
Based on what I saw before, the test is not very realistic in measuring contended performance and any spin-waiting in ConcurrentQueue.Enqueue/Dequeue would have the effect of turning this benchmark into a more single-threaded benchmark and its perf benefits from that. The large gain in the microbenchmark from #44265 also coincided with most of the large regressions in #45716. From the perf data in #46120 it seems clear that the change works well, even though it regresses this perf test.
Given this can we get a work item cut to get rid of these tests and put in place more realistic ones? Doesn't seem super useful to have around if when they regress we can't take any meaningful action.
Yea it would probably be reasonable to remove those tests. The ASP.NET tests seem to be better and seem to sufficiently cover this type of change for ConcurrentQueue
at least, but not sure about the other Concurrent*
tests. It's not easy to create realistic concurrent tests esp. in BDN style tests, I typically don't use BDN for those types of tests.
Actually it might be ok to keep the tests. They may not be good at assessing policy changes like in spin-waiting, but they could still identify regressions in pure overhead not involving policy changes. For a policy-type change we can just consider it a re-baseline since it's actually measuring something different from before.
Closing, as these are one-off differences and we can re-baseline on the new numbers
50 % regression in System.Collections.Concurrent.AddRemoveFromSameThreads.ConcurrentQueue and 4 other concurrent data structure benchmarks.
Based on the time stamps and changes, it looks like the only plausible cause for this is: https://github.com/dotnet/runtime/commit/8e6415dd1c0efc112cbd93cc58b3e87bc2b4ad1c
@kouvel Could that change have caused this regression?
Although it could theoretically be anything else in this date range: https://github.com/dotnet/runtime/compare/8e913a2a8157602af80cb6d3dda9391f28f11ba3...acd4855de9be7b51c853c4e5c719aa456a4865c9
Run Information
Regressions in System.Collections.Concurrent.AddRemoveFromSameThreads
Related Issue on x64 Windows
[Perf 54%] System.Collections.Concurrent.AddRemoveFromSameThreads.ConcurrentQueue
Related Issue on x86 Windows
[Perf 55%] System.Collections.Concurrent.AddRemoveFromSameThreads.ConcurrentQueue
Historical Data in Reporting System
Repro
Run Information
Regressions in System.Buffers.Tests.RentReturnArrayPoolTests
Related Issue on x64 Windows
[Perf -49%] System.Buffers.Tests.RentReturnArrayPoolTests.SingleSerial
Related Issue on x64 Windows
[Perf 30%] System.Buffers.Tests.RentReturnArrayPoolTests.MultipleSerial
Related Issue on x86 Windows
[Perf 66%] System.Buffers.Tests.RentReturnArrayPoolTests.SingleParallel
Related Issue on x86 Windows
[Perf 41%] System.Buffers.Tests.RentReturnArrayPoolTests (6)
Historical Data in Reporting System
Repro
Run Information
Regressions in System.Collections.Concurrent.AddRemoveFromSameThreads
Related Issue on x64 Windows
[Perf 55%] System.Collections.Concurrent.AddRemoveFromSameThreads.ConcurrentQueue
Related Issue on x86 Windows
[Perf 55%] System.Collections.Concurrent.AddRemoveFromSameThreads.ConcurrentQueue
Historical Data in Reporting System
Repro
Run Information
Regressions in System.Threading.Tests.Perf_ThreadPool
Related Issue on x64 Windows
[Perf 25%] System.Threading.Tests.Perf_ThreadPool.QueueUserWorkItem_WaitCallback_Throughput
Historical Data in Reporting System
Repro