Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo #22479

benaadams · 2019-02-07T23:44:17Z

Rather than a custom implementation, use the Vectorized SpanHelpers.SequenceCompareTo for

int CompareOrdinalHelper(string strA, string strB)

As per (which already uses it)

int CompareOrdinalHelper(string strA, int indexA, int countA, string strB, int indexB, int countB)

Performance should be additionally improved after #22187

/cc @jkotas @stephentoub

stephentoub · 2019-02-08T00:59:37Z

src/System.Private.CoreLib/shared/System/String.Comparison.cs

-                Debug.Assert(*(a + 1) != *(b + 1), "This char must be different if we reach here!");
-                return *(a + 1) - *(b + 1);
-            }
+            return SpanHelpers.SequenceCompareTo(ref strA.GetRawStringData(), strA.Length, ref strB.GetRawStringData(), strB.Length);


Does this have any impact positive or negative on perf, in particular for short strings?

Also, I believe call sites to this were already checking the first character. Is that still needed? Is the assert above still valid?

I love the reduction in code, just want to make sure we understand any unanticipated consequences.

Cleaned up the call sites as they were making more calls to the throw check than they needed to; but they are big switch methods, so is doubtful they will inline (which the comment it about), the _firstChar check is without bounds check so quite light, so probably remains an easy optimization to skip the method call; if its a common case.

Will get some numbers.

Actually one of the callsites might inline; but then the first char check would also inline, but SequenceCompareTo definitely won't; so it still works.

On #22187, changing char in last position of string

Method | Length | Mean | Error | StdDev | ---------------------- |------- |----------:|----------:|----------:| - string_CompareOrdinal | 1 | 3.204 ns | 3.5938 ns | 0.1970 ns | + string_CompareOrdinal | 1 | 3.159 ns | 0.2917 ns | 0.0160 ns | - string_CompareOrdinal | 4 | 6.343 ns | 0.2986 ns | 0.0164 ns | + string_CompareOrdinal | 4 | 9.657 ns | 0.6693 ns | 0.0367 ns | - string_CompareOrdinal | 5 | 6.561 ns | 0.4839 ns | 0.0265 ns | + string_CompareOrdinal | 5 | 6.955 ns | 1.4324 ns | 0.0785 ns | - string_CompareOrdinal | 6 | 6.611 ns | 0.4435 ns | 0.0243 ns | + string_CompareOrdinal | 6 | 7.945 ns | 0.4340 ns | 0.0238 ns | - string_CompareOrdinal | 7 | 7.400 ns | 0.4771 ns | 0.0262 ns | + string_CompareOrdinal | 7 | 8.733 ns | 0.6226 ns | 0.0341 ns | - string_CompareOrdinal | 8 | 7.438 ns | 0.4543 ns | 0.0249 ns | + string_CompareOrdinal | 8 | 7.501 ns | 0.1890 ns | 0.0104 ns | - string_CompareOrdinal | 15 | 6.256 ns | 0.2924 ns | 0.0160 ns | + string_CompareOrdinal | 15 | 8.128 ns | 0.1217 ns | 0.0067 ns | - string_CompareOrdinal | 16 | 6.232 ns | 0.2279 ns | 0.0125 ns | + string_CompareOrdinal | 16 | 6.999 ns | 0.2556 ns | 0.0140 ns | - string_CompareOrdinal | 24 | 9.363 ns | 0.3676 ns | 0.0201 ns | + string_CompareOrdinal | 24 | 7.577 ns | 0.4276 ns | 0.0234 ns | - string_CompareOrdinal | 31 | 9.098 ns | 0.2410 ns | 0.0132 ns | + string_CompareOrdinal | 31 | 7.594 ns | 0.4550 ns | 0.0249 ns | - string_CompareOrdinal | 32 | 9.105 ns | 0.5410 ns | 0.0297 ns | + string_CompareOrdinal | 32 | 7.578 ns | 0.2211 ns | 0.0121 ns | - string_CompareOrdinal | 33 | 10.227 ns | 0.1747 ns | 0.0096 ns | + string_CompareOrdinal | 33 | 8.418 ns | 0.3742 ns | 0.0205 ns | - string_CompareOrdinal | 63 | 9.809 ns | 0.4018 ns | 0.0220 ns | + string_CompareOrdinal | 63 | 9.311 ns | 0.7240 ns | 0.0397 ns | - string_CompareOrdinal | 64 | 9.833 ns | 0.0420 ns | 0.0023 ns | + string_CompareOrdinal | 64 | 9.340 ns | 2.9041 ns | 0.1592 ns | - string_CompareOrdinal | 127 | 15.538 ns | 0.7282 ns | 0.0399 ns | + string_CompareOrdinal | 127 | 12.648 ns | 1.3036 ns | 0.0715 ns | - string_CompareOrdinal | 128 | 15.628 ns | 1.0243 ns | 0.0561 ns | + string_CompareOrdinal | 128 | 12.595 ns | 0.3935 ns | 0.0216 ns | - string_CompareOrdinal | 255 | 23.341 ns | 0.9011 ns | 0.0494 ns | + string_CompareOrdinal | 255 | 19.403 ns | 0.2229 ns | 0.0122 ns | - string_CompareOrdinal | 256 | 23.223 ns | 0.3965 ns | 0.0217 ns | + string_CompareOrdinal | 256 | 19.455 ns | 0.5873 ns | 0.0322 ns | - string_CompareOrdinal | 1023 | 85.174 ns | 4.8935 ns | 0.2682 ns | + string_CompareOrdinal | 1023 | 73.952 ns | 0.7607 ns | 0.0417 ns | - string_CompareOrdinal | 1024 | 85.041 ns | 7.0647 ns | 0.3872 ns | + string_CompareOrdinal | 1024 | 73.840 ns | 5.9904 ns | 0.3284 ns |

The new numbers generally look worse for <= 16. Is that expected?

Yes currently as that's when vectors kick in; though I think I can turn it around.

Will wait till decision made on #22187 though as its complicated to try a coordinate across the two PRs and that's an improvement on current and I don't want to keep adding changes on changes as its already quite big.

i.e. SequenceCompareTo does char by char; this does long/int unrolls knowing alignment of string; however should be able to xor then find first changed char

benaadams · 2019-02-08T02:15:12Z

@dotnet-bot test CentOS7.1 x64 Debug Innerloop Build please

benaadams · 2019-02-08T02:54:29Z

Gave it a little push, since its just a wrapper method now

Total bytes of diff: -670 (-0.02% of base)
    diff is an improvement.

Total byte diff includes 0 bytes from reconciling methods
        Base had    1 unique methods,       32 unique bytes
        Diff had    1 unique methods,       32 unique bytes

Top file improvements by size (bytes):
        -670 : System.Private.CoreLib.dasm (-0.02% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
          32 : System.Private.CoreLib.dasm - String:ThrowIfStringComparisonInvalid(int) (0/1 methods)
          22 : System.Private.CoreLib.dasm - String:CompareOrdinal(ref,ref):int

Top method improvements by size (bytes):
        -204 : System.Private.CoreLib.dasm - String:CompareOrdinalHelper(ref,ref):int
        -107 : System.Private.CoreLib.dasm - String:Compare(ref,ref,int):int
        -100 : System.Private.CoreLib.dasm - String:EndsWith(ref,int):bool:this
         -97 : System.Private.CoreLib.dasm - String:Equals(ref,int):bool:this
         -93 : System.Private.CoreLib.dasm - String:Equals(ref,ref,int):bool
         -91 : System.Private.CoreLib.dasm - String:StartsWith(ref,int):bool:this
         -32 : System.Private.CoreLib.dasm - String:CheckStringComparison(int) (1/0 methods)

9 total methods with size differences (7 improved, 2 regressed), 17368 unchanged.

benaadams · 2019-02-08T07:17:07Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

stephentoub · 2019-03-15T16:42:22Z

@benaadams, are you still working on this? The PR you mentioned in "Will wait till decision made on #22187" has been closed.

benaadams · 2019-04-07T15:18:12Z

Will follow up after #22505

maryamariyan · 2019-11-06T21:05:26Z

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

In your coreclr repository clone, create patch by running git format-patch origin
In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

jkotas · 2019-11-09T19:31:52Z

src/System.Private.CoreLib/shared/System/String.Comparison.cs

@@ -56,6 +56,8 @@ private static bool EqualsOrdinalIgnoreCase(string strA, string strB)

            return CompareInfo.EqualsOrdinalIgnoreCase(ref strA.GetRawStringData(), ref strB.GetRawStringData(), strB.Length);
        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private static unsafe int CompareOrdinalHelper(string strA, string strB)


This helper does not seem to be providing much value. Would it be better to just inline the call in the two places that this is caller, without all the Debug.Asserts that describe the custom contract?

jkotas · 2019-11-09T19:34:54Z

Could you please share the up to date perf numbers?

benaadams · 2019-11-09T20:42:03Z

Could you please share the up to date perf numbers?

I'm having trouble building atm

The "GetLastStablePackage" task failed unexpectedly.
error MSB4018: System.IO.FileLoadException: Could not load file or assembly 'NuGet.Versioning, Version=5.3.0.4, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. Could not find or load a specific file. (0x80131621)
error MSB4018: File name: 'NuGet.Versioning, Version=5.3.0.4, Culture=neutral, PublicKeyToken=31bf3856ad364e35' [C:\GitHub\coreclr\src.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]

BUILD: Building Packages for Windows_NT.x64.Checked
C:\Program Files\dotnet\sdk\5.0.100-alpha1-014713\MSBuild.dll /nologo -maxcpucount /m -verbosity:m /v:minimal /bl:C:\GitHub\coreclr\bin\Logs\Nuget_Windows_NT__x64__Checked.binlog /clp:Summary /nr:True /nodeReuse:false /p:ContinuousIntegrationBuild=False /p:TreatWarningsAsErrors=true /p:Configuration=Debug /p:RepoRoot=C:\GitHub\coreclr /p:Restore=True /p:DeployDeps=False /p:Build=True /p:Rebuild=False /p:Deploy=False /p:Test=False /p:Pack=False /p:IntegrationTest=False /p:PerformanceTest=False /p:Sign=False /p:Publish=False /p:PortableBuild=true /p:Platform=x64 /p:__BuildOS=Windows_NT /p:__BuildType=Checked /p:__BuildArch=x64 /p:RestoreDuringBuild=true /p:Projects=C:\GitHub\coreclr\src\.nuget\packages.builds /warnaserror C:\Users\thund\.nuget\packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.19558.11\tools\Build.proj
  Restore completed in 43.57 ms for C:\Users\thund\.nuget\packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.19558.11\tools\Tools.proj.
  Restore completed in 15.87 ms for C:\GitHub\coreclr\src\.nuget\Microsoft.NETCore.ILDAsm\Microsoft.NETCore.ILDAsm.builds.
  Restore completed in 15.88 ms for C:\GitHub\coreclr\src\.nuget\packages.builds.
  Restore completed in 15.76 ms for C:\GitHub\coreclr\src\.nuget\Microsoft.NETCore.Native\Microsoft.NETCore.Native.builds.
  Restore completed in 15.93 ms for C:\GitHub\coreclr\src\.nuget\Microsoft.NETCore.Jit\Microsoft.NETCore.Jit.builds.
  Restore completed in 15.81 ms for C:\GitHub\coreclr\src\.nuget\Microsoft.NETCore.TestHost\Microsoft.NETCore.TestHost.builds.
  Restore completed in 15.93 ms for C:\GitHub\coreclr\src\.nuget\Microsoft.NETCore.Crossgen2\Microsoft.NETCore.Crossgen2.builds.
  Restore completed in 15.78 ms for C:\GitHub\coreclr\src\.nuget\Microsoft.NETCore.ILAsm\Microsoft.NETCore.ILAsm.builds.
  Restore completed in 15.86 ms for C:\GitHub\coreclr\src\.nuget\Microsoft.NETCore.Runtime.CoreCLR\Microsoft.NETCore.Runtime.CoreCLR.builds.
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018: The "GetLastStablePackage" task failed unexpectedly. [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018: System.IO.FileLoadException: Could not load file or assembly 'NuGet.Versioning, Version=5.3.0.4, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. Could not find or load a specific file. (0x80131621) [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018: File name: 'NuGet.Versioning, Version=5.3.0.4, Culture=neutral, PublicKeyToken=31bf3856ad364e35' [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018:    at Microsoft.DotNet.Build.Tasks.Packaging.GetLastStablePackage.GetLastStablePackagesFromIndex() [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018:    at Microsoft.DotNet.Build.Tasks.Packaging.GetLastStablePackage.Execute() in /_/src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetLastStablePackage.cs:line 59 [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018:  [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018:  [C:\GitHub\coreclr\src\.nuget\Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018: The "GetLastStablePackage" task failed unexpectedly. [C:\GitHub\coreclr\src\.nuget\Microsoft.TargetingPack.Private.CoreCLR\Microsoft.TargetingPack.Private.CoreCLR.pkgproj]
C:\Users\thund\.nuget\packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.19558.11\build\Packaging.targets(389,5): error MSB4018: System.IO.FileLoadException: Could not load file or assembly 'NuGet.Versioning, Version=5.3.0.4, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. Could not find or load a specific file. (0x80131621) [C:\GitHub\coreclr\src\.nuget\Microsoft.TargetingPack.Private.CoreCLR\Microsoft.TargetingPack.Private.CoreCLR.pkgproj]

maryamariyan · 2019-12-02T19:38:43Z

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.

stephentoub reviewed Feb 8, 2019

View reviewed changes

benaadams changed the title ~~Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo~~ [WIP] Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo Feb 8, 2019

jkotas added area-System.Runtime area-System.Memory and removed area-System.Runtime labels Nov 2, 2019

benaadams force-pushed the CompareOrdinalHelper branch from 70f6706 to b6c0488 Compare November 9, 2019 18:27

benaadams changed the title ~~[WIP] Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo~~ Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo Nov 9, 2019

Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo

2c5a7b3

benaadams force-pushed the CompareOrdinalHelper branch from b6c0488 to 2c5a7b3 Compare November 9, 2019 19:13

jkotas reviewed Nov 9, 2019

View reviewed changes

jkotas approved these changes Nov 9, 2019

View reviewed changes

feedback

6d79a3b

benaadams mentioned this pull request Nov 30, 2019

Use SpanHelpers.SequenceCompareTo instead of CompareOrdinalHelper dotnet/runtime#402

Closed

maryamariyan closed this Dec 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo #22479

Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo #22479

benaadams commented Feb 7, 2019 •

edited

Loading

stephentoub Feb 8, 2019

benaadams Feb 8, 2019

benaadams Feb 8, 2019

benaadams Feb 8, 2019

stephentoub Feb 8, 2019

benaadams Feb 8, 2019

benaadams Feb 8, 2019

benaadams commented Feb 8, 2019

benaadams commented Feb 8, 2019

benaadams commented Feb 8, 2019

stephentoub commented Mar 15, 2019

benaadams commented Apr 7, 2019

maryamariyan commented Nov 6, 2019

jkotas Nov 9, 2019

jkotas commented Nov 9, 2019

benaadams commented Nov 9, 2019

maryamariyan commented Dec 2, 2019

Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo #22479

Use CompareOrdinalHelper for SpanHelpers.SequenceCompareTo #22479

Conversation

benaadams commented Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Feb 8, 2019

benaadams commented Feb 8, 2019

benaadams commented Feb 8, 2019

stephentoub commented Mar 15, 2019

benaadams commented Apr 7, 2019

maryamariyan commented Nov 6, 2019

Choose a reason for hiding this comment

jkotas commented Nov 9, 2019

benaadams commented Nov 9, 2019

maryamariyan commented Dec 2, 2019

benaadams commented Feb 7, 2019 •

edited

Loading