Skip to content
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 System.Globalization.Tetsts.StringSearch #50736

Closed
DrewScoggins opened this issue Apr 5, 2021 · 12 comments
Closed

[Perf] Regressions in System.Globalization.Tetsts.StringSearch #50736

DrewScoggins opened this issue Apr 5, 2021 · 12 comments
Labels
arch-x64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI os-linux Linux OS (any supported distro) tenet-performance Performance related issue tenet-performance-benchmarks Issue from performance benchmark
Milestone

Comments

@DrewScoggins
Copy link
Member

Run Information

Architecture x64
OS ubuntu 18.04
Baseline ca1a5cf09bc2e286c24029f205d376831af1e383
Compare dc5c9099b2c4ebcc74f8f40a5de5c4733ac79bfa

Regressions in System.Globalization.Tests.StringSearch

Benchmark Baseline Test Test/Base Baseline IR Compare IR IR Ratio Baseline ETL Compare ETL
IndexOf_Word_NotFound 507.56 ns 598.57 ns 1.18
IndexOf_Word_NotFound 508.54 ns 597.88 ns 1.18
LastIndexOf_Word_NotFound 503.82 ns 599.36 ns 1.19
IndexOf_Word_NotFound 509.19 ns 600.15 ns 1.18
LastIndexOf_Word_NotFound 502.60 ns 599.06 ns 1.19
LastIndexOf_Word_NotFound 502.24 ns 599.30 ns 1.19

graph
graph
graph
graph
graph
graph
![graph]
Historical Data in Reporting System

Repro

git clone https://github.com/dotnet/performance.git
python3 .\performance\scripts\benchmarks_ci.py -f netcoreapp5.0 --filter 'System.Globalization.Tests.StringSearch*'

Payloads

Baseline
Compare

Histogram

System.Globalization.Tests.StringSearch.IndexOf_Word_NotFound(Options: (en-US, None, False))


System.Globalization.Tests.StringSearch.IsPrefix_FirstHalf(Options: (, IgnoreCase, False))


System.Globalization.Tests.StringSearch.IndexOf_Word_NotFound(Options: (en-US, IgnoreNonSpace, False))


System.Globalization.Tests.StringSearch.LastIndexOf_Word_NotFound(Options: (en-US, IgnoreNonSpace, False))


System.Globalization.Tests.StringSearch.IndexOf_Word_NotFound(Options: (, None, False))


System.Globalization.Tests.StringSearch.LastIndexOf_Word_NotFound(Options: (en-US, None, False))


System.Globalization.Tests.StringSearch.LastIndexOf_Word_NotFound(Options: (, None, False))


System.Globalization.Tests.StringSearch.IsPrefix_FirstHalf(Options: (en-US, IgnoreCase, False))


Docs

Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository

@DrewScoggins DrewScoggins added os-linux Linux OS (any supported distro) tenet-performance Performance related issue tenet-performance-benchmarks Issue from performance benchmark arch-x64 labels Apr 5, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Globalization untriaged New issue has not been triaged by the area owner labels Apr 5, 2021
@ghost
Copy link

ghost commented Apr 5, 2021

Tagging subscribers to this area: @tarekgh, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Run Information

Architecture x64
OS ubuntu 18.04
Baseline ca1a5cf09bc2e286c24029f205d376831af1e383
Compare dc5c9099b2c4ebcc74f8f40a5de5c4733ac79bfa

Regressions in System.Globalization.Tests.StringSearch

Benchmark Baseline Test Test/Base Baseline IR Compare IR IR Ratio Baseline ETL Compare ETL
IndexOf_Word_NotFound 507.56 ns 598.57 ns 1.18
IndexOf_Word_NotFound 508.54 ns 597.88 ns 1.18
LastIndexOf_Word_NotFound 503.82 ns 599.36 ns 1.19
IndexOf_Word_NotFound 509.19 ns 600.15 ns 1.18
LastIndexOf_Word_NotFound 502.60 ns 599.06 ns 1.19
LastIndexOf_Word_NotFound 502.24 ns 599.30 ns 1.19

graph
graph
graph
graph
graph
graph
![graph]
Historical Data in Reporting System

Repro

git clone https://github.com/dotnet/performance.git
python3 .\performance\scripts\benchmarks_ci.py -f netcoreapp5.0 --filter 'System.Globalization.Tests.StringSearch*'

Payloads

Baseline
Compare

Histogram

System.Globalization.Tests.StringSearch.IndexOf_Word_NotFound(Options: (en-US, None, False))


System.Globalization.Tests.StringSearch.IsPrefix_FirstHalf(Options: (, IgnoreCase, False))


System.Globalization.Tests.StringSearch.IndexOf_Word_NotFound(Options: (en-US, IgnoreNonSpace, False))


System.Globalization.Tests.StringSearch.LastIndexOf_Word_NotFound(Options: (en-US, IgnoreNonSpace, False))


System.Globalization.Tests.StringSearch.IndexOf_Word_NotFound(Options: (, None, False))


System.Globalization.Tests.StringSearch.LastIndexOf_Word_NotFound(Options: (en-US, None, False))


System.Globalization.Tests.StringSearch.LastIndexOf_Word_NotFound(Options: (, None, False))


System.Globalization.Tests.StringSearch.IsPrefix_FirstHalf(Options: (en-US, IgnoreCase, False))


Docs

Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository

Author: DrewScoggins
Assignees: -
Labels:

arch-x64, area-System.Globalization, os-linux, tenet-performance, tenet-performance-benchmarks, untriaged

Milestone: -

@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Apr 5, 2021
@tarekgh tarekgh added this to the 6.0.0 milestone Apr 5, 2021
@tarekgh
Copy link
Member

tarekgh commented Apr 5, 2021

@stephentoub

I have quickly scanned all commits between the baseline and the regression:

dc5c9099b2c [metadata] Don't access MonoTableInfo:rows, use table_info_get_rows() (#49738)
bc821616706 [wasm] Add more information about windows build (#50103)
dddac294c61 Add Span overloads to System.Numerics.Vectors (#50062)
51d6023c6b6 update branding to preview4 (#50049)
e64237dbc19 [main] Update dependencies from dotnet/xharness dotnet/llvm-project (#50086)
e279550ac2e Guard use of brew command by OSX (#50069)
49a73e3e5ce Use the one-shot hash functions for asymmetric operations (#50061)
5001157773c Gate enum tests that depend on Ref.Emit (#50093)
f690d59f4ea Don't remove trailing `\0` in custom attributes (#50088)
33f0398e006 Mark ActiveIssue on Vector test which is still failing (#50091)
ce0a530d7ed Move GetExceptionForHRInternal into common (#50085)
184a5b103ca Enable CA1052 (static holder types should be static) (#50047)

The only one looks related is 184a5b1. Is it expected to get some regression with this change?

@stephentoub
Copy link
Member

No, that should have zero impact.

@stephentoub
Copy link
Member

Is it possible there were updates to the machines in this timeframe, e.g. Windows Update patches that may have been applied? If you run the two versions of the repo again on the same machine, does it still manifest as a repeatable regression?

@tarekgh
Copy link
Member

tarekgh commented Apr 5, 2021

No, that should have zero impact.

Interesting, I am not seeing any other commit can be related at all.

Is it possible there were updates to the machines in this timeframe, e.g. Windows Update patches that may have been applied?

This regression looks on Linux only. but I am not sure if these Linux machines get updated somehow.

If you run the two versions of the repo again on the same machine, does it still manifest as a repeatable regression?

@DrewScoggins is this something you can try?

CC @adamsitnik

@DrewScoggins
Copy link
Member Author

Yes, I can run these on the same machine. I will do that today and report back the numbers.

@DrewScoggins
Copy link
Member Author

So I ran these and did see the regression. I ran it for the other ones as well, but I am not posting them here because as you can see from the image I pasted below this regression went away with this change, #50633, from Andy. So I suspect this had something to do with the application of PGO data.

Baseline

[2021/04/06 10:31:08][INFO] |                Method |                           Options |        Mean |      Error |     StdDev |      Median |         Min |         Max | Gen 0 | Gen 1 | Gen 2 | Allocated |
[2021/04/06 10:31:08][INFO] |---------------------- |---------------------------------- |------------:|-----------:|-----------:|------------:|------------:|------------:|------:|------:|------:|----------:|
[2021/04/06 10:31:08][INFO] | IndexOf_Word_NotFound |                   (, None, False) |   513.37 ns |   8.898 ns |   8.323 ns |   507.67 ns |   506.92 ns |   530.06 ns |     - |     - |     - |         - |
[2021/04/06 10:31:08][INFO] | IndexOf_Word_NotFound |    (en-US, IgnoreNonSpace, False) |   517.83 ns |   4.373 ns |   4.090 ns |   519.62 ns |   508.17 ns |   520.34 ns |     - |     - |     - |         - |
[2021/04/06 10:31:08][INFO] | IndexOf_Word_NotFound |              (en-US, None, False) |   512.60 ns |   6.182 ns |   5.782 ns |   509.01 ns |   507.04 ns |   519.74 ns |     - |     - |     - |         - |

Compare

[2021/04/06 10:34:39][INFO] |                Method |                           Options |        Mean |      Error |    StdDev |      Median |         Min |         Max | Gen 0 | Gen 1 | Gen 2 | Allocated |
[2021/04/06 10:34:39][INFO] |---------------------- |---------------------------------- |------------:|-----------:|----------:|------------:|------------:|------------:|------:|------:|------:|----------:|
[2021/04/06 10:34:39][INFO] | IndexOf_Word_NotFound |                   (, None, False) |   598.04 ns |   2.216 ns |  1.851 ns |   597.32 ns |   596.67 ns |   603.16 ns |     - |     - |     - |         - |
[2021/04/06 10:34:39][INFO] | IndexOf_Word_NotFound |    (en-US, IgnoreNonSpace, False) |   597.50 ns |   0.942 ns |  0.835 ns |   597.05 ns |   596.70 ns |   599.46 ns |     - |     - |     - |         - |
[2021/04/06 10:34:39][INFO] | IndexOf_Word_NotFound |              (en-US, None, False) |   604.36 ns |   7.035 ns |  6.580 ns |   607.62 ns |   597.16 ns |   611.07 ns |     - |     - |     - |         - |

image

@tarekgh
Copy link
Member

tarekgh commented Apr 6, 2021

CC @AndyAyersMS to confirm this may be related PGO.

@DrewScoggins thanks for the info. Is this means we are currently not having the regression anymore? should we have seen any changes related to PGO that caused the problem and then @AndyAyersMS changes fixed it back?

@AndyAyersMS
Copy link
Member

CC @AndyAyersMS to confirm this may be related PGO.

Seems likely.

PGO data started appearing in FX assemblies on 3/19 with #49793 (first bump in the graphs up top) and that caused some regressions. I don't think we root-caused the second bump on 3/23 (IIRC there was no obvious commit).

The jit wasn't properly handling the PGO data in some cases; that's what lead to some of those 3/19 regressions. I partially fixed this in #50633. There is still more to fix.

@stephentoub
Copy link
Member

@DrewScoggins, were there other issues opened that were likely also the result of this?

@tarekgh tarekgh added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Globalization labels Jun 15, 2021
@tarekgh
Copy link
Member

tarekgh commented Jun 15, 2021

@AndyAyersMS I changed the label to codegen as we got a new issue #54165 which is regression in same test highly likely a codegen issue.

@AndyAyersMS
Copy link
Member

Ok, thanks. The regressions in the tests above were all resolved over time -- the one for #54165 still persists.

So I'm going to close this.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI os-linux Linux OS (any supported distro) tenet-performance Performance related issue tenet-performance-benchmarks Issue from performance benchmark
Projects
None yet
Development

No branches or pull requests

4 participants