Repository navigation
[Perf] Regressions in System.Buffers.Tests.ReadOnlySequenceTests<Char> #52312
Description
Run Information
| Architecture | x64 |
|---|---|
| OS | Windows 10.0.18362 |
| Baseline | ddaa1c3c503f989db48102f50a693765e4e6ae2a |
| Compare | 8c3180d932146b1a76600601c03082c6a81e1a53 |
| Diff | Diff |
Regressions in System.Buffers.Tests.ReadOnlySequenceTests<Char>
| Benchmark | Baseline | Test | Test/Base | Test Quality | Baseline IR | Compare IR | IR Ratio | Baseline ETL | Compare ETL |
|---|---|---|---|---|---|---|---|---|---|
| FirstSpanTenSegments - Duration of single invocation | 21.75 ns | 22.89 ns | 1.05 | 0.00 | |||||
| FirstSpanArray - Duration of single invocation | 10.22 ns | 14.64 ns | 1.43 | 0.01 | |||||
| FirstSpanMemory - Duration of single invocation | 11.59 ns | 16.02 ns | 1.38 | 0.01 |
Historical Data in Reporting System
_1.png)
_2.png)
_3.png)
Repro
git clone https://github.com/dotnet/performance.git
py .\performance\scripts\benchmarks_ci.py -f netcoreapp5.0 --filter 'System.Buffers.Tests.ReadOnlySequenceTests<Char>*'Details
Payloads
Histogram
System.Buffers.Tests.ReadOnlySequenceTests<Char>.FirstSpanTenSegments
System.Buffers.Tests.ReadOnlySequenceTests<Char>.FirstSpanArray
System.Buffers.Tests.ReadOnlySequenceTests<Char>.FirstSpanMemory
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
Activity
GrabYourPitchforks commentedon Jul 6, 2021
I checked the history for https://github.com/dotnet/runtime/commits/main/src/libraries/System.Memory and don't see any changes between the baseline & compare commits.
Could this be caused by #51593?
ReadOnlySequence<T>goes a bit wild with inlining, so anything that affects how inlining works may have outsized impact on this set of APIs. Moving to codegen area for additional investigation.kunalspathak commentedon Jul 6, 2021
It might be worth checking if #51190 affects any of the generated code for these benchmarks.
GrabYourPitchforks commentedon Jul 6, 2021
#51190 would definitely affect codegen, but it was about removing
movsxinstructions in favor of zero-latencymovinstructions (or even being able to elide them entirely). I'll double-check the codegen to make sure nothing strange got introduced.DrewScoggins commentedon Jul 7, 2021
All of these tests have improved with this change, #52708. You can look at the tests by clicking on the links in the table, and see. But I am going to go ahead and close this since we are no longer seeing the regression.
4 remaining items
kunalspathak commentedon Jul 7, 2021
@GrabYourPitchforks - I am assigning this to you for now. Once you have data and you think it is a codegen issue, feel free to assign back to me.
GrabYourPitchforks commentedon Jul 7, 2021
Using a baseline of 1983573 and a compare of de591a8, there's something strange going on with the method prolog for this function.
I changed that function to vary the number of
consume += sequence.First.Length;statements which appear. (The original benchmark has 16 such statements.) The number of stack bytes carved out in the method prolog (viasub rsp, <xxx>) is as follows.0x400x400x680x680x880x880xB80xB80xD80xD80xF80xF80x1180xF8A few interesting items here. First, there's a jump of
0x30instead of0x20between statement counts 3 & 4. Second, I think there's opportunity for the JIT to reuse temp locals between invocations of the inlined methodReadOnlySequence<T>.GetFirstSpan(), which would further reduce the required stack space across the board. Finally, at statement count = 7, there's a weird discrepancy between the baseline and compare.Up until sequence count = 7, the codegen looks fine for both the baseline and the compare. The only difference between the two methods is that some
movsxdstatements in the baseline got converted intomovstatements in the compare, saving 1 byte of codegen each time such a conversion was replaced.Kunul, punting this back your way because I don't know how to investigate this further.
kunalspathak commentedon Jul 9, 2021
I cherry-picked each commit in 508e560...466deef and compared the disassembly generated. It turns out that there are lot of regressions in
FirstSpanfrom @GrabYourPitchforks 's de591a8 changes. The diffs are here. I will further investigate.GrabYourPitchforks commentedon Jul 12, 2021
@kunalspathak Right, this matches what I saw as well. The codegen for
ReadOnlySequence<T>.GetFirstSpan()itself is nearly identical, modulo somemovsx/movdifferences. (This is expected since that was the entire point of the commit.) That method is being inlined into its caller in the benchmark class, which executes it over and over again. Things still look fine until the method is inlined 7 or more times, at which point something goes amiss and the codegen diverges considerably. That part I don't know how to debug.kunalspathak commentedon Jul 13, 2021
Since most of the
Unsafe.Add(byref,int)were converted toUnsafe.Add(byref,nint), we did not do constant folding which was earlier possible.For such variables, when we inlined the methods, we started introducing extra temps (18 extras, to be precise) as seen in the variable assignments screenshot below:
Because of these, we hit the local variable limit and stop promoting the structs after one point causing the inefficient code gen.
The regression is amplified because there are 16 calls to
sequence.FirstSpan.Lengthwhich is not something that a real-world app would do. I also verified the benchmark's disasm and dumps on latest main and they are no longer present because we do not create as many locals - My guess is because of the Egor's inline change, we do better job in inlining/eliminating redundant variables or other optimizations.To summarize:
I will close this issue tomorrow unless anyone else have any other comments.
BruceForstall commentedon Jul 13, 2021
The default tracked variable limit is 1024. It turns out this is configurable with COMPlus_JitMaxLocalsToTrack. Extensive measurements were made when it was upped to this (from something much smaller -- 64 I think), and larger than this there was little CQ benefit but growing throughput cost.
EgorBo commentedon Jul 13, 2021
I incremented it in #52708
It's not difficult to hit the limit of 512, e.g. sharplab.io, even some TE benchmark hit that, but I don't remember which exactly - I can check if it's important.
Also, I noticed this issue several times in SIMD-related code written by community - e.g. lots of Unsafe.X here and there, lots of large methods with AggressiveInlining on them led to those terrible spills.
I didn't measure TP impact for this change, but I did measure it for the whole PR (mainly via "startup" and "time-to-first request" metrics and it was mostly tolerable).
Btw, setting the limit to 1024 decreases the size of prejitted CoreLib by 20kb (I can share the diffs)