-
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
Regressions in System.Buffers.Binary.Tests.BinaryReadAndWriteTests #85988
Comments
Tagging subscribers to this area: @dotnet/area-system-buffers Issue DetailsRun Information
Regressions in System.Buffers.Binary.Tests.BinaryReadAndWriteTests
ReproGeneral Docs link: https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md Payloadsgit clone https://github.com/dotnet/performance.git
py .\performance\scripts\benchmarks_ci.py -f net8.0 --filter 'System.Buffers.Binary.Tests.BinaryReadAndWriteTests*' PayloadsHistogramSystem.Buffers.Binary.Tests.BinaryReadAndWriteTests.MeasureReverseUsingNtoH
Description of detection logic
JIT DisasmsSystem.Buffers.Binary.Tests.BinaryReadAndWriteTests.MeasureReverseEndianness
Description of detection logic
JIT DisasmsDocsProfiling workflow for dotnet/runtime repository
|
Commit range is 7afd85d...edd6d63. Probably #85654, @jakobbotsch? Link to the benchmark. Similar stuff in #85994. |
Seems unlikely, #85654 had no diffs related to those benchmarks. But I don't see how the other commits would affect those benchmarks either. |
Also could be #85559 -- perhaps, some empty arrays got moved and so perturbed the alignment of the benchmark array? |
This one is worth investigating more. It remains almost 1.5x slower with no clear culprit. |
I'll take a closer look. |
Yes this is from #85559, cc @EgorBo. We end up putting a static array field used in the micro benchmark on the non-GC heap, but then we constant propagate its address into a loop and regress performance of the loop. I wonder if we can generally avoid propagating from low weight blocks to high weight blocks, although it's probably not that simple. It also doesn't seem feasible to fix this in 8.0, so I will move this to 9.0. -Importing BB01 (PC=000) of 'System.Buffers.Binary.Tests.BinaryReadAndWriteTests:MeasureReverseUsingNtoH():int[]:this'
- [ 0] 0 (0x000) ldsfld 04000B90
-Checking if we can import 'static readonly' as a jit-time constant...
- [ 1] 5 (0x005) stloc.0Querying runtime about current class of field <unknown class>:<unknown field> (declared as int[])
-Runtime reports field is init-only and initialized and has class int[]
-
-lvaUpdateClass: Updating class for V01 from (00007FFAE8373060) int[] to (00007FFAE8373060) int[] [exact]
-
-
-STMT00000 ( 0x000[E-] ... ??? )
- [000003] -A--G------ ▌ ASG ref
- [000002] D------N--- ├──▌ LCL_VAR ref V01 loc0
- [000001] #---G------ └──▌ IND ref
- [000000] H---------- └──▌ CNS_INT(h) long 0x1f0ccc06c08 const ptr Fseq[<unknown field>]
+Importing BB01 (PC=000) of 'System.Buffers.Binary.Tests.BinaryReadAndWriteTests:MeasureReverseUsingNtoH():int[]:this'
+ [ 0] 0 (0x000) ldsfld 04000B90
+Checking if we can import 'static readonly' as a jit-time constant... ... success! The value is:
+ [000000] H---------- ▌ CNS_INT(h) ref
+
+ [ 1] 5 (0x005) stloc.0
+
+STMT00000 ( 0x000[E-] ... ??? )
+ [000002] -A--------- ▌ ASG ref
+ [000001] D------N--- ├──▌ LCL_VAR ref V01 loc0
+ [000000] H---------- └──▌ CNS_INT(h) ref
|
I'll actually leave this in 8.0 for a bit, maybe someone else has an idea about something we can do. |
I think we should do something here, but I do not see any possible surgical fix. I think ideally this is fixed by 1) avoiding this propagation of constants into loops/higher weighted blocks in the first place, and 2) some rematerialization support for constants to not regress register allocation when we do 1. |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Moving to 10 for the same reason as the above. We sadly did not get around to revisiting some of the necessities for improving cases like this one. |
Run Information
Regressions in System.Buffers.Binary.Tests.BinaryReadAndWriteTests
Test Report
Repro
General Docs link: https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md
Payloads
Baseline
Compare
Payloads
Baseline
Compare
Histogram
System.Buffers.Binary.Tests.BinaryReadAndWriteTests.MeasureReverseUsingNtoH
Description of detection logic
JIT Disasms
Baseline
Compare
Diff
System.Buffers.Binary.Tests.BinaryReadAndWriteTests.MeasureReverseEndianness
Description of detection logic
JIT Disasms
Baseline
Compare
Diff
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
The text was updated successfully, but these errors were encountered: