-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[Perf] Regressions in Dictionary and Hashset on 9/24/2022 1:05:07 PM #76256
Comments
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
Seems related to #75663, @stephentoub. We are seeing regressions in a large number of Dictionary and Hashset performance tests across all configurations as well. |
Do current builds include PGO updates based on my change? |
They do not. We can look at this again once we have updated PGO data, if you think that is a big factor here. |
I suspect it is. We'll see. If it ends up persisting after a PGO update, I'll revert and take another whack at it. |
Tagging subscribers to this area: @dotnet/area-system-collections Issue DetailsRun Information
Regressions in System.Collections.TryGetValueFalse<Int32, Int32>
Reprogit clone https://github.com/dotnet/performance.git
python3 .\performance\scripts\benchmarks_ci.py -f net6.0 --filter 'System.Collections.TryGetValueFalse<Int32, Int32>*' PayloadsHistogramEdge Detector InfoSystem.Collections.TryGetValueFalse<Int32, Int32>.ConcurrentDictionary(Size: 512)
Description of detection logic
Description of detection logic
Description of detection logic
DocsProfiling workflow for dotnet/runtime repository
|
I'm still working on this. I'm getting very close to having it running on .NET 8.0 preview 2. There's an issue that makes preview 3 and newer fail on ARM. I'm working on a fix for that also, but current priority is to get us up to date with at least preview 2. |
The .NET 8.0 preview 2 update is complete, and PGO/IBC packages have been submitted to darc for the next round of dependency updates. |
@EgorBo, I'm not sure what to make of this issue. There are two tests here, one for Dictionary and one for IDictionary. The Dictionary one returned to normal, but the IDictionary one didn't, yet the IDictionary test is exactly the same as the one for Dictionary just going through its IDictionary interface. Ideas? |
I still blame PGO here, I guess it's still not here. Win-x64 default: Same machine but with |
That latter picture is for Dictionary, not IDictionary. Also, dynamic pgo would enable devirtualization via IDictionary in the test... static wouldn't, such that they're not comparable, right? |
The point that I don't see this regression on the runs with PGO enabled. For both |
But dynamic pgo would make the Dictionary and IDictionary tests effectively equivalent. Static pgo wouldn't, so I'm not understanding. |
It's not necessary that it's devirtualization that kicks in here, any change to C# impl also makes overall static profile stale until it's regenerated. Stale profile can trigger regressions for sure. From what I see, currently our repo still uses static PGO that was trained on November 2022's version of dotnet/runtime code, so it's definitely stale. |
Ok, I guess we continue to wait for everything to propagate. We need to find a way to close this gap. @sblom, what is the expected time between when a change is checked into dotnet/runtime main and that change's impact is reflected in pgo data also merged into runtime main? From your most recent comment about "Preview 2", am I to infer that we plan to only do so once a month? |
#83624 PR updates PGO data but it also does not yet include the commit we need (it brings |
I think under normal circumstances, the biggest delay between a code change and its contribution to profile data will be mediated by Maestro. We can take runtime update PRs from dotnet-bot pretty much continuously, and from watching the last cycle, it looks like once we've published profiles, Mastro will PR them into runtime main within about a week. (I'm admittedly less clear on that part.)
I think it'll be more frequent than that. The reason we're on Preview 2 isn't policy--it's that I'll work on getting latest main to work. |
Got it. Thanks. |
I got a preview 4 build through the pipeline this afternoon. Profiles should get picked up by maestro's next pr into runtime. Let me know if you see anything that looks wrong. |
Excellent. Thank you. |
Remaining regression went away. Looks like there might be a new one, but that'll be covered by a different issue. |
These two tests, at least, were also regressed by this change. However, they did not improve after updated PGO data was pushed through. |
@stephentoub can you take another look and see if there is anything we still need to do here to address a potential regression? |
I don't think there's anything further actionable here. Thanks. |
Run Information
Regressions in System.Collections.TryGetValueFalse<Int32, Int32>
Test Report
Repro
Payloads
Baseline
Compare
Histogram
Edge Detector Info
Collection Data
System.Collections.TryGetValueFalse<Int32, Int32>.ConcurrentDictionary(Size: 512)
Description of detection logic
Description of detection logic
Description of detection logic
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
The text was updated successfully, but these errors were encountered: