-
Notifications
You must be signed in to change notification settings - Fork 4
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] Linux/x64: 6 Regressions on 8/28/2024 6:34:28 PM #40587
Comments
I think it was dotnet/runtime#106994. fyi: @jkoritzinsky |
cc: @carlossanlop looks like upgrading Brotli may have introduced some regressions. What do you think is the best course of action here? |
Are those regressions expected according to the brotli update release notes or the owners? The recent zlib-ng perf regressions were accepted because they were clearly described by the zlib-ng owners as expected. |
The release notes for brotli are pretty bare bones. They didn’t describe any known regressions. https://github.com/google/brotli/releases/tag/v1.1.0 Here’s a link to more regressions as well as some improvements from the upgrade: dotnet/runtime#107302 (comment) |
You could ask the brotli owners if they're aware of this perf regression in their tests. If they can justify it, then we could probably take the update if no one objects. I did that with the zlib-ng owners and they provided detailed explanations to justify some of the perf regressions. Also, it does not look like this brotli update contains any security fixes, but I am unsure if they publish them in the release tag notes. If we are absolutely sure there were no security fixes, should we just skip this update until we get significant enough improvements that would justify the perf regression? Any thoughts on my suggestions, @jkotas? |
Do we understand what caused the improvements in May 2024 and July 2024? It sounds like that this benchmark may be sensitive to unrelated factors.
I do not think we can afford the stay behind. 15% regression is not prohibitive. If you include the earlier improvements, it is ~7% for release-to-release comparison. |
Do we see these regressions on CoreCLR as well? |
The only commit in range for May is : dotnet/runtime#101630 |
It looks like we see some of the regressions on CoreCLR as well, as recorded in #107302 and the linked regressions. There's also some improvements for some of the benchmarks linked there. Really weird grouping of improvements vs regressions though at a high level glance. |
We may want to include |
I agree with @jkotas that we cannot afford to stay behind. I also agree with @carlossanlop that we should notify the maintainers of brotli about the performance differences we see between 1.0.9 and 1.1.0. @jkoritzinsky can you summarize the differences we see in a way that might be informative or actionable to the brotli owners? |
@ericstj I opened an issue in google/brotli with our results to try to understand the regressions: google/brotli#1202 |
@jkotas I can open an issue to track your request, but in which repo is this done? runtime or performance? |
@jkotas I opened dotnet/runtime#108073 but if the correct repo was dotnet/performance, please feel free to move it there. Let's close this as we're taking the brotli update regardless, and the follow ups have been addressed (opening an issue for google/brotli to continue the regression conversation, and open the PGO issue). |
I don't have permission to close issues in this repo. @jkurdek can you please help? |
Run Information
Regressions in System.IO.Compression.Brotli
Test Report
Repro
General Docs link: https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md
Repro Steps
Prerequisites (Build files either built locally or downloaded from payload above)
runtime/artifacts
or build instructions: Libraries README args:-subset libs+libs.tests -rc release -configuration Release -arch $RunArch -framework net8.0
runtime/artifacts/bin/coreclr/$RunOS.$RunArch.Release
, build instructions: CoreCLR README args:-subset clr+libs -rc release -configuration Release -arch $RunArch -framework net8.0
runtime/artifacts/bin/mono/$RunOS.$RunArch.Release
, build instructions: MONO README args:-arch $RunArch -os $RunOS -s mono+libs+host+packs -c Release
Linux
Windows
System.IO.Compression.Brotli.Compress_WithoutState(level: Optimal, file: "TestDocument.pdf")
ETL Files
Histogram
JIT Disasms
System.IO.Compression.Brotli.Compress_WithState(level: Optimal, file: "TestDocument.pdf")
ETL Files
Histogram
JIT Disasms
System.IO.Compression.Brotli.Compress_WithState(level: Optimal, file: "alice29.txt")
ETL Files
Histogram
JIT Disasms
System.IO.Compression.Brotli.Compress_WithoutState(level: Optimal, file: "alice29.txt")
ETL Files
Histogram
JIT Disasms
System.IO.Compression.Brotli.Compress_WithoutState(level: Optimal, file: "sum")
ETL Files
Histogram
JIT Disasms
System.IO.Compression.Brotli.Compress_WithState(level: Optimal, file: "sum")
ETL Files
Histogram
JIT Disasms
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
The text was updated successfully, but these errors were encountered: