Adding Span LastIndexOfAny APIs and tests#25848
Conversation
|
Is this PR good to merge? @dotnet-bot test Linux x64 Release Build |
| IntPtr index = (IntPtr)(uint)length; // Use UIntPtr for arithmetic to avoid unnecessary 64->32->64 truncations | ||
| IntPtr nLength = (IntPtr)(uint)length; | ||
| #if !netstandard11 | ||
| if (Vector.IsHardwareAccelerated && length >= Vector<byte>.Count * 2) |
There was a problem hiding this comment.
Some more comments in this code would be useful.
There was a problem hiding this comment.
I won't block on this (since CI is green), but will keep a note of this, especially if we were to re-work some of the implementation details.
| if ((int)(byte*)index > 0) | ||
| { | ||
| nLength = index; | ||
| goto SequentialScan; |
There was a problem hiding this comment.
If you go to sequential scan from here, after finishing the sequential scan, you will execute if (Vector.IsHardwareAccelerated && ((int)(byte*)index > 0)) gain, which will never evaluate to true, i.e. you will execute the check twice. Is this the best way to organize the code?
There was a problem hiding this comment.
Yes, you are right. Good catch!
We should consider looking at all the {Last}IndexOf vectorized APIs together and see if we can optimize it further since such issues exist in existing implementation too.
I had discussed this previously with Levi. I will file an issue.
There was a problem hiding this comment.
* Adding LastIndexOfAny APIs and tests. * Adding ReadOnlySpan LastIndexOfAny tests. * Remove service property from test project * Remove service property from performance test project Commit migrated from dotnet/corefx@c20881a
3 participants
Fixes https://github.com/dotnet/corefx/issues/24839
Related PR #25748
cc @benaadams, @KrzysztofCwalina, @GrabYourPitchforks, @jkotas, @stephentoub