Vectorize string.IndexOf(..., StringComparison.Ordinal) #21076

benaadams · 2018-11-19T02:38:31Z

x 2.5 times faster at position 0
x 11.2 times faster at position 1024

For

// where comparisonType == StringComparison.Ordinal 
string.IndexOf(string value, StringComparison comparisonType)
string.IndexOf(string value, int startIndex, StringComparison comparisonType)
string.IndexOf(string value, int startIndex, int count, StringComparison comparisonType)

// where T : char
IndexOf<T>(this Span<T> span, ReadOnlySpan<T> value)
IndexOf<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> value)

// where comparisonType == StringComparison.Ordinal
IndexOf(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparisonType)

// where options = CompareOptions.Ordinal 
CompareInfo.IndexOf(string source, string value, CompareOptions options)
CompareInfo.IndexOf(string source, string value, int startIndex, CompareOptions options)
CompareInfo.IndexOf(string source, string value, int startIndex, int count, CompareOptions options)

https://gist.github.com/benaadams/177d989fea110f51fab14cd708239852

           Method | Position |      Mean |
 ---------------- |--------- |----------:|
- IndexOf_Ordinal |        0 |  35.86 ns |
+ IndexOf_Ordinal |        0 |  14.15 ns |
- IndexOf_Ordinal |        7 |  42.69 ns |
+ IndexOf_Ordinal |        7 |  19.00 ns |
- IndexOf_Ordinal |        8 |  43.13 ns |
+ IndexOf_Ordinal |        8 |  18.90 ns |
- IndexOf_Ordinal |       15 |  49.67 ns |
+ IndexOf_Ordinal |       15 |  19.61 ns |
- IndexOf_Ordinal |       16 |  47.58 ns |
+ IndexOf_Ordinal |       16 |  19.64 ns |
- IndexOf_Ordinal |       31 |  60.92 ns |
+ IndexOf_Ordinal |       31 |  21.09 ns |
- IndexOf_Ordinal |       32 |  62.66 ns |
+ IndexOf_Ordinal |       32 |  21.11 ns |
- IndexOf_Ordinal |       63 | 100.10 ns |
+ IndexOf_Ordinal |       63 |  22.45 ns |
- IndexOf_Ordinal |       64 | 100.82 ns |
+ IndexOf_Ordinal |       64 |  22.54 ns |
- IndexOf_Ordinal |      127 | 156.94 ns |
+ IndexOf_Ordinal |      127 |  27.10 ns |
- IndexOf_Ordinal |      128 | 155.85 ns |
+ IndexOf_Ordinal |      128 |  26.07 ns |
- IndexOf_Ordinal |      255 | 265.79 ns |
+ IndexOf_Ordinal |      255 |  32.93 ns |
- IndexOf_Ordinal |      256 | 266.91 ns |
+ IndexOf_Ordinal |      256 |  32.98 ns |
- IndexOf_Ordinal |     1023 | 918.89 ns |
+ IndexOf_Ordinal |     1023 |  81.13 ns |
- IndexOf_Ordinal |     1024 | 916.90 ns |
+ IndexOf_Ordinal |     1024 |  81.20 ns |

Contributes to https://github.com/dotnet/coreclr/issues/19672

/cc @jkotas, @krwq, @tarekgh

benaadams · 2018-11-19T02:40:48Z

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs

-        // TODO: Instead of this method could we just have upstack code call IndexOfOrdinal with ignoreCase = false?
-        private static unsafe int FastIndexOfString(string source, string target, int startIndex, int sourceCount, int targetCount, bool findLastIndex)
+        // TODO: Instead of this method could we just have upstack code call LastIndexOfOrdinal with ignoreCase = false?
+        private static unsafe int FastLastIndexOfString(string source, string target, int startIndex, int sourceCount, int targetCount)


Just removed the forward index, last index is still handled here. Ignoring whitespace shows removal more clearly https://github.com/dotnet/coreclr/pull/21076/files?w=1

benaadams · 2018-11-19T04:20:21Z

OSX10.12 infra issue: xunit/xunit#1855

benaadams · 2018-11-19T04:21:08Z

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

src/System.Private.CoreLib/shared/System/MemoryExtensions.Fast.cs

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.cs

src/System.Private.CoreLib/shared/System/MemoryExtensions.Fast.cs

benaadams · 2018-11-19T08:28:53Z

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

benaadams · 2018-11-19T10:14:46Z

OSX Fail

.GetFileSizeEx: ERROR -> SetEndOfFile failed due to lack of disk space

benaadams · 2018-11-19T11:06:17Z

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

benaadams · 2018-11-19T13:42:33Z

OSX Fail

.GetFileSizeEx: ERROR -> SetEndOfFile failed due to lack of disk space

benaadams · 2018-11-19T14:54:15Z

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

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Unix.cs

benaadams · 2018-11-19T17:33:22Z

I think OSX CI is generally broken

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs

tarekgh · 2018-11-19T17:38:17Z

CC @dotnet/dnceng for OSX leg failures

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs

krwq · 2018-11-19T17:51:08Z

I assume there already are test cases checking for all of the edge cases we could hit here?

MattGal · 2018-11-19T18:31:07Z

@benaadams @tarekgh : note that running out of disk space on an OSX machine does not make OSX CI generally broken since after finishing their current Jenkins agent job these machines will in most cases (except when jusssssst enough cleanup happens after the agent finishes) will not take more work. In the retry case, the machine its running on currently still has 18 GB free space and will hopefully succeed.

Jenkins in general rarely cleans up workspaces, so it's a constant battle to clean up these machines.

GrabYourPitchforks · 2018-11-19T18:30:16Z

src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs

+
+                // Found the first element of "value". See if the tail matches.
+                if (SequenceEqual(
+                    ref Unsafe.As<char, byte>(ref Unsafe.Add(ref searchSpace, index + 1)),


Is it ever possible for index to be Int32.MaxValue, causing this addition to integer overflow? I think the answer is no (because IndexOf should never return that value) but am having trouble convincing myself of this.

Rearranged to avoid overflow

Also the byte version

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.cs

2 callers expect value.Length == 0 to be tested first 3rd caller ReplaceCore doesn't allow a value.Length == 0 so moving it first is ok

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.cs

jkotas · 2018-11-21T00:50:41Z

Thank you!

…lr#21076) * Vectorize string.IndexOf(..., StringComparison.Ordinal) * Merge two IndexOf(..., int* matchLengthPtr) * fix 2 callers expect value.Length == 0 to be tested first 3rd caller ReplaceCore doesn't allow a value.Length == 0 so moving it first is ok Commit migrated from dotnet/coreclr@b526aff

Vectorize string.IndexOf(..., StringComparison.Ordinal)

87c9578

benaadams commented Nov 19, 2018

View reviewed changes

This was referenced Nov 19, 2018

Use TZCNT and LZCNT for Locate{First|Last}Found{Byte|Char} #21073

Merged

System.InvalidOperationException: Collection was modified after the enumerator was instantiated. xunit/xunit#1855

Closed

jkotas reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/MemoryExtensions.Fast.cs Show resolved Hide resolved

jkotas reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/MemoryExtensions.Fast.cs Show resolved Hide resolved

jkotas reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.cs Show resolved Hide resolved

feedback

ea0bb5e

jkotas reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/MemoryExtensions.Fast.cs Outdated Show resolved Hide resolved

fix

939bebf

tarekgh approved these changes Nov 19, 2018

View reviewed changes

krwq reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Unix.cs Outdated Show resolved Hide resolved

krwq reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs Outdated Show resolved Hide resolved

krwq reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs Show resolved Hide resolved

krwq reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs Outdated Show resolved Hide resolved

krwq reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs Outdated Show resolved Hide resolved

GrabYourPitchforks reviewed Nov 19, 2018

View reviewed changes

jkotas reviewed Nov 19, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs Outdated Show resolved Hide resolved

benaadams force-pushed the Vectorize-string.IndexOf(-,-StringComparison.Ordinal) branch from 2252666 to 66b681d Compare November 20, 2018 04:39

benaadams added 3 commits November 20, 2018 04:39

feedback

9698051

Rearrange to avoid overflow

66b681d

Only single subtraction needed

0e65a01

jkotas reviewed Nov 20, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.Windows.cs Outdated Show resolved Hide resolved

jkotas reviewed Nov 20, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.cs Outdated Show resolved Hide resolved

Merge two IndexOf(..., int* matchLengthPtr)

9532029

jkotas approved these changes Nov 20, 2018

View reviewed changes

benaadams added 2 commits November 20, 2018 07:20

fix

5911c3e

fix 2

00be952

2 callers expect value.Length == 0 to be tested first 3rd caller ReplaceCore doesn't allow a value.Length == 0 so moving it first is ok

benaadams commented Nov 20, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.cs Show resolved Hide resolved

benaadams commented Nov 20, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Globalization/CompareInfo.cs Show resolved Hide resolved

jkotas merged commit b526aff into dotnet:master Nov 21, 2018

benaadams deleted the Vectorize-string.IndexOf(-,-StringComparison.Ordinal) branch November 21, 2018 01:13

Vectorize string.IndexOf(..., StringComparison.Ordinal) #21076

Vectorize string.IndexOf(..., StringComparison.Ordinal) #21076

Uh oh!

Conversation

benaadams commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

benaadams commented Nov 19, 2018

Uh oh!

benaadams commented Nov 19, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benaadams commented Nov 19, 2018

Uh oh!

benaadams commented Nov 19, 2018

Uh oh!

benaadams commented Nov 19, 2018

Uh oh!

benaadams commented Nov 19, 2018

Uh oh!

benaadams commented Nov 19, 2018

Uh oh!

Uh oh!

benaadams commented Nov 19, 2018

Uh oh!

Uh oh!

Uh oh!

tarekgh commented Nov 19, 2018

Uh oh!

Uh oh!

Uh oh!

krwq commented Nov 19, 2018

Uh oh!

MattGal commented Nov 19, 2018

Uh oh!

GrabYourPitchforks Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

benaadams Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams Nov 20, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas commented Nov 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

benaadams commented Nov 19, 2018 •

edited

Loading

benaadams Nov 20, 2018 •

edited

Loading