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 -116%] System.Memory.ReadOnlySpan.IndexOfString (4) #37815

Closed
DrewScoggins opened this issue Jun 12, 2020 · 5 comments
Closed

[Perf -116%] System.Memory.ReadOnlySpan.IndexOfString (4) #37815

DrewScoggins opened this issue Jun 12, 2020 · 5 comments
Labels
Milestone

Comments

@DrewScoggins
Copy link
Member

DrewScoggins commented Jun 12, 2020

Run Information

Architecture x64
OS Windows 10.0.18362
Changes diff

Regressions in System.Memory.ReadOnlySpan

Benchmark Baseline Test Test/Base Modality Baseline Outlier
IndexOfString 270.81 ns 851.49 ns 3.14 False
IndexOfString 41.60 ns 60.54 ns 1.46 True
IndexOfString 49.03 ns 105.97 ns 2.16 True
IndexOfString 58.53 ns 112.59 ns 1.92 True

graph
graph
graph
graph
Historical Data in Reporting System

Repro

git clone https://github.com/dotnet/performance.git
py .\performance\scripts\benchmarks_ci.py -f netcoreapp5.0 --filter 'System.Memory.ReadOnlySpan*';

Histogram

System.Memory.ReadOnlySpan.IndexOfString(input: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAXAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", value: "x", comparisonType: OrdinalIgnoreCase)

[250.798 ; 321.811) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[321.811 ; 392.825) | 
[392.825 ; 463.838) | 
[463.838 ; 534.851) | 
[534.851 ; 605.865) | 
[605.865 ; 676.878) | 
[676.878 ; 747.891) | 
[747.891 ; 799.121) | 
[799.121 ; 870.134) | @@@@@@@@@@@@@@@@
[870.134 ; 894.531) | 
[894.531 ; 965.544) | @@@@

System.Memory.ReadOnlySpan.IndexOfString(input: "StrIng", value: "string", comparisonType: OrdinalIgnoreCase)

[31.924 ; 34.905) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[34.905 ; 37.599) | @
[37.599 ; 40.139) | 
[40.139 ; 41.715) | @@@@@@
[41.715 ; 44.409) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[44.409 ; 45.934) | @@@@@@
[45.934 ; 48.628) | 
[48.628 ; 51.322) | 
[51.322 ; 52.515) | 
[52.515 ; 55.958) | @
[55.958 ; 58.780) | @@@@
[58.780 ; 61.474) | @@@@@@@@@@@@@
[61.474 ; 63.528) | @@

System.Memory.ReadOnlySpan.IndexOfString(input: "Hello WorldbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbareallyreallylongHello WorldbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbareallyreallylongHello Worldbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbareallyreallylong!xyz", value: "w", comparisonType: OrdinalIgnoreCase)

[ 36.991 ;  43.673) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[ 43.673 ;  48.230) | 
[ 48.230 ;  54.912) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[ 54.912 ;  61.594) | 
[ 61.594 ;  68.276) | 
[ 68.276 ;  74.958) | 
[ 74.958 ;  81.640) | 
[ 81.640 ;  88.322) | 
[ 88.322 ;  94.924) | 
[ 94.924 ; 100.936) | @@
[100.936 ; 107.618) | @@@@@@@@@@@@@@@@@@

System.Memory.ReadOnlySpan.IndexOfString(input: "More Test's", value: "Tests", comparisonType: OrdinalIgnoreCase)

[ 47.992 ;  54.920) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[ 54.920 ;  57.783) | 
[ 57.783 ;  64.711) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[ 64.711 ;  71.639) | 
[ 71.639 ;  78.568) | 
[ 78.568 ;  85.496) | 
[ 85.496 ;  92.424) | 
[ 92.424 ;  99.352) | 
[ 99.352 ; 104.219) | 
[104.219 ; 111.937) | @@
[111.937 ; 118.865) | @@@@@@@@@@@@@@
[118.865 ; 127.125) | @@@@

Docs

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

@DrewScoggins DrewScoggins added the tenet-performance-benchmarks Issue from performance benchmark label Jun 12, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Memory untriaged New issue has not been triaged by the area owner labels Jun 12, 2020
@DrewScoggins
Copy link
Member Author

From @adamsitnik

Looks like a regression. A bad one (OrdinalIgnoreCase shoud not be using ICU and should always be very fast). @GrabYourPitchforks

@GrabYourPitchforks
Copy link
Member

I suspect this is related to the ICU work. We have managed optimizations for Equals(..., OrdinalIgnoreCase), but we never had managed optimizations for IndexOf(..., OrdinalIgnoreCase). So it's likely that calling into ICU for this scenario is more expensive than calling into NLS.

Do we know if this same regression exists when we opt back in to using NLS everywhere? It can be toggled via an environment variable or by using a runtimeconfig.json file, as shown at https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization.Extensions/tests/NlsTests/runtimeconfig.template.json.

@adamsitnik adamsitnik added tenet-performance Performance related issue and removed tenet-performance-benchmarks Issue from performance benchmark untriaged New issue has not been triaged by the area owner labels Jul 6, 2020
@adamsitnik adamsitnik added this to the 5.0.0 milestone Jul 6, 2020
@DrewScoggins DrewScoggins added the tenet-performance-benchmarks Issue from performance benchmark label Jul 7, 2020
@DrewScoggins
Copy link
Member Author

I have updated the data to give more historical context. From looking at it I am fairly certain we would see these go away with the removal of ICU, as you can see that the test was quite flat and unregressed until that change went in.

@tarekgh
Copy link
Member

tarekgh commented Jul 10, 2020

#37919

@adamsitnik
Copy link
Member

I've confirmed that this regression has been caused by the switch to ICU:

Method input value comparisonType 3.1 Mean 5.0 ICU 5.0 NLS
IndexOfString AAAAA5AAAA 5 InvariantCulture 79.07 ns 35.41 ns 72.05 ns
IndexOfString AAAAAAAAAAAA(...)AAAAAAAAAAAA [1000] X Ordinal 39.72 ns 38.89 ns 38.25 ns
IndexOfString AAAAAAAAAAAA(...)AAAAAAAAAAAA [100] x InvariantCultureIgnoreCase 300.45 ns 214.39 ns 294.59 ns
IndexOfString AAAAAAAAAAAA(...)AAAAAAAAAAAA [100] x OrdinalIgnoreCase 245.94 ns 802.33 ns 240.42 ns
IndexOfString ABCDE c InvariantCultureIgnoreCase 71.81 ns 30.26 ns 63.83 ns
IndexOfString Hello Worldb(...)allylong!xyz [186] w OrdinalIgnoreCase 45.80 ns 102.63 ns 40.30 ns
IndexOfString Hello Worldb(...)allylong!xyz [187] ~ Ordinal 28.52 ns 22.28 ns 21.03 ns
IndexOfString Hello Worldbb(...)bbbbbbbbbbba! [47] y Ordinal 24.15 ns 14.09 ns 13.57 ns
IndexOfString More Test's Tests OrdinalIgnoreCase 56.38 ns 108.93 ns 50.48 ns
IndexOfString StrIng string OrdinalIgnoreCase 38.79 ns 55.01 ns 33.86 ns
IndexOfString foobardzsdzs rddzs InvariantCulture 101.46 ns 44.93 ns 94.90 ns
IndexOfString string1 string2 InvariantCulture 91.45 ns 37.14 ns 80.95 ns
IndexOfString ? ? InvariantCulture 108.70 ns 4,069.11 ns 105.36 ns
IndexOfString ????????????(...)???????????? [100] ? Ordinal 25.08 ns 16.88 ns 15.48 ns
IndexOfString ????????????(...)???????????? [1000] x Ordinal 39.70 ns 39.19 ns 37.99 ns

I am closing this issue and adding it to the list of all known ICU-related regressions: #40942

@adamsitnik adamsitnik changed the title [Perf -116%] System.Memory.ReadOnlySpan (4) [Perf -116%] System.Memory.ReadOnlySpan.IndexOfString (4) Aug 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants