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] Changes at 11/17/2021 1:35:01 AM #61821

Closed
performanceautofiler bot opened this issue Nov 18, 2021 · 8 comments · Fixed by #61935
Closed

[Perf] Changes at 11/17/2021 1:35:01 AM #61821

performanceautofiler bot opened this issue Nov 18, 2021 · 8 comments · Fixed by #61935

Comments

@performanceautofiler
Copy link

Run Information

Architecture arm64
OS Windows 10.0.19041
Baseline c66fa996b5cc32d94b2e6a8dfeb7c3d3a3098694
Compare da0e0f73e24036a88411dde8158df80e5f4bff01
Diff Diff

Regressions in System.Globalization.Tests.StringSearch

Benchmark Baseline Test Test/Base Test Quality Edge Detector Baseline IR Compare IR IR Ratio Baseline ETL Compare ETL
IsPrefix_DifferentFirstChar - Duration of single invocation 8.45 ns 19.29 ns 2.28 0.24 True
IsSuffix_DifferentLastChar - Duration of single invocation 8.25 ns 18.97 ns 2.30 0.24 True

graph
graph
Test Report

Repro

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

Payloads

Baseline
Compare

Histogram

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


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


Docs

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

@DrewScoggins DrewScoggins transferred this issue from dotnet/perf-autofiling-issues Nov 19, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Globalization untriaged New issue has not been triaged by the area owner labels Nov 19, 2021
@ghost
Copy link

ghost commented Nov 19, 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 arm64
OS Windows 10.0.19041
Baseline c66fa996b5cc32d94b2e6a8dfeb7c3d3a3098694
Compare da0e0f73e24036a88411dde8158df80e5f4bff01
Diff Diff

Regressions in System.Globalization.Tests.StringSearch

Benchmark Baseline Test Test/Base Test Quality Edge Detector Baseline IR Compare IR IR Ratio Baseline ETL Compare ETL
IsPrefix_DifferentFirstChar - Duration of single invocation 8.45 ns 19.29 ns 2.28 0.24 True
IsSuffix_DifferentLastChar - Duration of single invocation 8.25 ns 18.97 ns 2.30 0.24 True

graph
graph
Test Report

Repro

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

Payloads

Baseline
Compare

Histogram

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


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


Docs

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

Author: performanceautofiler[bot]
Assignees: -
Labels:

area-System.Globalization, untriaged

Milestone: -

@DrewScoggins DrewScoggins added arch-arm64 os-windows tenet-performance Performance related issue tenet-performance-benchmarks Issue from performance benchmark and removed area-System.Globalization untriaged New issue has not been triaged by the area owner labels Nov 19, 2021
@DrewScoggins
Copy link
Member

Seems related to #61640

@safern
Copy link
Member

safern commented Nov 19, 2021

cc: @AaronRobinsonMSFT @jkoritzinsky as it seems related to the DllImport Generator change.

@AaronRobinsonMSFT
Copy link
Member

/cc @elinor-fung

@elinor-fung
Copy link
Member

Were any of the converted p/invokes in SPCL marshalling strings as ANSI?

@AaronRobinsonMSFT
Copy link
Member

@elinor-fung was able to root cause this issue. She will be providing details on her investigation shortly.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 22, 2021
@elinor-fung
Copy link
Member

Stepped through CompareInfo.IsPrefix and CompareInfo.IsSuffix with the input from the regressed test cases - they are scenarios that do not end up calling any p/invokes, which is actually why they are the ones that regressed.

With #61640, the StartsWith and EndsWith generated stubs and blittable p/invokes are now being inlined into their callers, leading to a p/invoke frame being initialized (JIT_InitPInvokeFrame) even for the scenarios that would not end up invoking it.

Before #61640:

Name                                                                       	Inc %	     Inc	Exc %	   Exc
+ System.Private.CoreLib.il!CompareInfo.IsPrefix                           	 63.6	   4,648	  8.8	   646
|+ System.Private.CoreLib.il!CompareInfo.IcuStartsWith                     	 53.4	   3,899	 22.7	 1,655
||+ System.Private.CoreLib.il!CompareInfo.StartsWithOrdinalIgnoreCaseHelper	 29.6	   2,159	 29.6	 2,159
||+ OTHER <<?!?>>                                                          	  1.2	      84	  1.2	    84

After:

Name                                                                       	Inc %	     Inc	Exc %	   Exc
+ System.Private.CoreLib.il!CompareInfo.IsPrefix                           	 62.1	   4,926	  6.1	   487
|+ System.Private.CoreLib.il!CompareInfo.IcuStartsWith                     	 54.9	   4,355	 15.3	 1,214
||+ System.Private.CoreLib.il!CompareInfo.StartsWithOrdinalIgnoreCaseHelper	 33.5	   2,652	 21.0	 1,666
|||+ coreclr!JIT_InitPInvokeFrame                                          	 12.4	     986	 12.4	   986
||+ coreclr!JIT_InitPInvokeFrame                                           	  6.0	     476	  6.0	   476
||+ OTHER <<?!?>>                                                          	  0.2	      12	  0.2	    12

@AaronRobinsonMSFT
Copy link
Member

It appears this is a know case where the JIT can do better. It is described/tracked in #59002.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants