[Perf] Regressions in System.Text.Perf_Utf8Encoding for Greek and Cyrillic #52313
Comments
DrewScoggins
added
os-linux
dotnet-issue-labeler
bot
added
area-System.Text.Encoding
untriaged
|
Tagging subscribers to this area: @tarekgh, @krwq, @eiriktsarpalis, @layomia Issue DetailsRun Information
Regressions in System.Text.Perf_Utf8Encoding
Reprogit clone https://github.com/dotnet/performance.git
py .\performance\scripts\benchmarks_ci.py -f netcoreapp5.0 --filter 'System.Text.Perf_Utf8Encoding*'PayloadsHistogramSystem.Text.Perf_Utf8Encoding.GetString(Input: Greek)System.Text.Perf_Utf8Encoding.GetString(Input: Cyrillic)System.Text.Perf_Utf8Encoding.GetBytes(Input: Greek)System.Text.Perf_Utf8Encoding.GetBytes(Input: Cyrillic)DocsProfiling workflow for dotnet/runtime repository
|
|
From @L2: Regarding System.Text.Perf_Utf8Encoding.GetBytes(Input: Cyrillic) |
|
cc @AndyAyersMS |
danmoseley
added
area-CodeGen-coreclr
|
Thanks @alexcovington |
JulieLeeMSFT
added
needs-further-triage
|
Looking a bit wider at the history for the first
The regression noted in this issue is the jump on 20-Apr. Subsequently we have made improvements and the net over the entire 6.0 cycle is about 2% regression over 5.0. however for the
So let's take a closer look at this second test. |
|
5.0 vs 6.0rc1 preview:
|
|
Also oddly full PGO doesn't help here... in fact it's a bit slower overall.
|
|
As noted above the jit crashes trying to dump and we die computing the node hash during morph: This may be fixed in more recent builds, will have to check. Yes, seems like it was fixed right after these changes, at #51627. |
|
Cause of the layout diffs is that some blocks are now (properly) weighted at zero: and so likely we reorder code in a way that negatively impacts Greek/Cyrillic as we never saw any static PGO runs that hit those code blocks. A possible fix here would be to update our MPGO collections to include some runs done in other locales. @davidwrighton does that seem worth trying at this stage in the release? |
|
I have no objection to adding an internationalization scenario, but @Lxiamail is the expert in that space. Technically we are basically in position to do this, its just a bit of work. Please provide a priority here. |
|
Area owners @tarekgh, @krwq, @eiriktsarpalis, @layomia can you provide some guidance? We have two options:
Longer term I think we should definitely add the coverage, the question is whether we should try doing it now. |
|
Does the regression is scoped to the Greek and Cyrillic cases only? From @AndyAyersMS comment #52313 (comment) it looks we have a regression with the ASCII cases too in the scenario
|
|
By the way, I am not the area owner, I am just expressing my opinion. area owners can still advise as needed. |
|
As a stopgap, is it worth considering suppressing profile-guided optimization of just this one family of methods? We'd still want all other JIT optimizations to kick in, just not PGO. |
There is currently no way to do that, unfortunately. |
|
Should we repurpose this tracking issue to cover adding the necessary non-ASCII tests for generating PGO data? One interesting aspect of the transcoding logic is that it's written with the understanding that its blocks won't be reordered. That is, the blocks within the method are already in an order which should give optimal throughput across multiple languages. Hence my question about suppressing PGO for this one family of methods. |
Seems prudent. We may also have to work to do to ensure that our PGO merging does reasonable things here.
There's never been any such guarantee made by the jit when optimizing; it reorders blocks as it deems best, and may duplicate blocks in places if it thinks such duplication will be helpful. It may be that the current jit tends not to reorder code much from IL order, but that will be less and less true as time goes on. In .NET 7 we'll likely add a blended synthetic profile to the static PGO data so we're a bit less dependent on PGO training covering all the key scenarios. But that is a ways off. |
Oh, definitely. :) I didn't intend to hold this up as an example of good practice. Just that it's another case where we do somewhat "fun" things in corelib because it ships alongside the JIT and manual inspection of the (corelib, JIT) combo [at least when this code was written] showed optimal codegen. |
How about this - I'll leave the current issue to track whatever JIT changes you think might be appropriate here, and I opened #57698 to track the specific action of feeding a new corpus into PGO. Sound good? |
Sure. We should update the PGO collection process for main and keep an eye on the perf impact there (takes a few days to know for sure). If we're happy with the results, we can make the case to back port this to .NET 6 and/or try and surgically update the MIBC somehow. |
|
Changing to .NET 7 to revisit after #57698. |
|
Would branching hints (RyuJIT: Allow developers to provide Branch Prediction Information) be a solution for this? That could be a long-term strategy to stabilize benchmarks. |
|
@EgorBo another case I hope improves with some non EN-US PGO data. |
|
Don't they look fine on the global picture? e.g. |
JulieLeeMSFT
removed
the
needs-further-triage
|
If you look back further, you will see we are regressed vs 5.0 in many cases. Hopefully the PGO updates that are coming will address most of these. BenchmarkDotNet=v0.13.1.1786-nightly, OS=Windows 11 (10.0.22000.795/21H2) PowerPlanMode=00000000-0000-0000-0000-000000000000 IterationTime=250.0000 ms MaxIterationCount=20
|
|
#73973 updated PGO data that includes adhoc scenarios for Encoding to train on non-latin data, as the result:
|
ghost
locked as resolved and limited conversation to collaborators
Run Information
Regressions in System.Text.Perf_Utf8Encoding
Historical Data in Reporting System
Repro
Payloads
Baseline
Compare
Histogram
System.Text.Perf_Utf8Encoding.GetString(Input: Greek)
System.Text.Perf_Utf8Encoding.GetString(Input: Cyrillic)
System.Text.Perf_Utf8Encoding.GetBytes(Input: Greek)
System.Text.Perf_Utf8Encoding.GetBytes(Input: Cyrillic)
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
The text was updated successfully, but these errors were encountered: