-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Audit MemoryExtensions.IndexOf variants #75754
Conversation
Tagging subscribers to this area: @dotnet/area-system-memory Issue DetailsHi @adamsitnik, after your #73768 I am submitting a few changes which I noticed and presumed were not intentionally left out:
Things I noticed but did not change, deciding they were intentional:
Sadly I am having a few troubles getting some benchmarks running. I will submit this as a draft until then, if you see anything I am doing obviously wrong I would appreciate the pointer (I am a rookie with the runtime repo). I built main branch and copied the artifacts folder out to
dotnet --info
benchmark src
|
public static int LastIndexOfAny<T>(this Span<T> span, ReadOnlySpan<T> values) where T : IEquatable<T>? | ||
{ | ||
if (Unsafe.SizeOf<T>() == sizeof(byte) && RuntimeHelpers.IsBitwiseEquatable<T>()) | ||
return SpanHelpers.LastIndexOfAny( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was calling the same function as the fallback
Benchmarks summary: Regressions in IndexOfAny_byte_ROS and LastIndexOfAny_byte_ROS for 2-value I guess the former is related to codegen. I will try a more representative search space soon. In the meantime if you have any thoughts on what changes you would take (if any) please let me know. Thanks BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22000.978/21H2)
AMD Ryzen 7 5700U with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET SDK=8.0.100-alpha.1.22465.14
[Host] : .NET 8.0.0 (8.0.22.46215), X64 RyuJIT AVX2
main : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PR : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
public class PrValidation
{
[Params(8, 64, 1024)]
public int SearchSpaceCount;
[Params(1, 2, 3, 4, 5)]
public int ValueSpaceCount;
private byte[] _searchSpaceBytes, _valueSpaceBytes;
private char[] _searchSpaceChars, _valueSpaceChars;
[GlobalSetup]
public void Setup()
{
_searchSpaceBytes = new byte[SearchSpaceCount];
_searchSpaceChars = new char[SearchSpaceCount];
_valueSpaceBytes = Enumerable.Range(1, ValueSpaceCount).Select(Convert.ToByte).ToArray();
_valueSpaceChars = Enumerable.Range(1, ValueSpaceCount).Select(Convert.ToChar).ToArray();
}
[Benchmark] public void LastIndexOf_char_Span() => new Span<char>(_searchSpaceChars).LastIndexOf(_valueSpaceChars);
[Benchmark] public void IndexOfAny_byte_ROS() => new ReadOnlySpan<byte>(_searchSpaceBytes).IndexOfAny(_valueSpaceBytes);
[Benchmark] public void LastIndexOfAny_byte_Span() => new Span<byte>(_searchSpaceBytes).LastIndexOfAny(_valueSpaceBytes);
[Benchmark] public void LastIndexOfAny_byte_ROS() => new ReadOnlySpan<byte>(_searchSpaceBytes).LastIndexOfAny(_valueSpaceBytes);
[Benchmark] public void LastIndexOfAny_char_Span() => new Span<char>(_searchSpaceChars).LastIndexOfAny(_valueSpaceChars);
[Benchmark] public void LastIndexOfAny_char_ROS() => new ReadOnlySpan<char>(_searchSpaceChars).LastIndexOfAny(_valueSpaceChars);
} |
I ran some slightly more comprehensive benchmarks on LastIndexOfAny 5-value. Really the changes are just unifying the Span and ReadOnlySpan versions which I presume is supposed to be the case and I am only submitting them in the context of correcting a mistake rather than any optimisation opportunity. So the benchmarks are just for completeness sake (and curiosity). BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22000.978/21H2)
AMD Ryzen 7 5700U with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET SDK=8.0.100-alpha.1.22465.14
[Host] : .NET 8.0.0 (8.0.22.46215), X64 RyuJIT AVX2
main : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
PR : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
public class LoremIpsum
{
[Params(10, 100, 1000)]
public int SearchSpaceCount;
private char[] _searchSpace;
[GlobalSetup]
public void Setup()
{
_searchSpace = _loremIpsum.AsSpan(0, SearchSpaceCount).ToArray();
}
[Benchmark]
[ArgumentsSource(nameof(ValueSpaces))]
public int LastIndexOfAny_Span(string valueSpace) => new Span<char>(_searchSpace).LastIndexOfAny(valueSpace);
[Benchmark]
[ArgumentsSource(nameof(ValueSpaces))]
public int LastIndexOfAny_ROS(string valueSpace) => new ReadOnlySpan<char>(_searchSpace).LastIndexOfAny(valueSpace);
public IEnumerable<object> ValueSpaces()
{
yield return "\r\n\t\\\""; // (-1, -1, 724) in _searchSpace.AsSpan(0, SearchSpaceCount)
yield return "abcde"; // (3, 96, 996)
yield return "zzzzu"; // (-1, 57, 899)
yield return "LWXYZ"; // (0, 0, 101)
}
private readonly string _loremIpsum = @"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam sodales orci at urna efficitur finibus. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Aenean ultrices facilisis volutpat. Curabitur vel sollicitudin risus. Nunc tortor libero, pretium ut mi eu, finibus rutrum enim. Vivamus malesuada vel nunc eu faucibus. Aliquam volutpat ultrices risus non pellentesque. Vivamus dignissim augue non dignissim pellentesque. Duis a fermentum dui. Nullam quis vestibulum neque. Quisque euismod ligula in ex blandit, sed aliquam orci lacinia. Curabitur facilisis, ante cursus venenatis dictum, elit ante rutrum nibh, vitae maximus libero odio ac est. Aliquam dignissim eros quis congue convallis. Aenean et erat libero.
Donec sapien nisl, efficitur nec arcu non, convallis fringilla nunc. Aenean rhoncus, tortor quis imperdiet placerat, dui nibh accumsan felis, sed mattis odio lectus sed est. Nulla cursus dictum sodales. Maecenas eget ornare augue, sed aliquam mauris. Fusce vel nisl congue, porta elit et, semper lectus.";
} |
This method is updated to match LastIndexOf(ROS, ROS)
This is consistent with IndexOfAny(Span, ROS)
70091fc
to
cc8bd94
Compare
ping @adamsitnik not sure if you saw this one (which was unfortunately timed amongst the other IndexOf fixes), or if you otherwise had any thoughts. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for a very thoughtful audit of all MemoryExtensions
overloads! And apologies for the delay in review.
The PR looks great to me, failures are unrelated. Merging!
@@ -1478,18 +1518,8 @@ private static unsafe int IndexOfAnyProbabilistic(ref char searchSpace, int sear | |||
/// </summary> | |||
/// <param name="span">The span to search.</param> | |||
/// <param name="values">The set of values to search for.</param> | |||
[MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rob-Hague @adamsitnik
Was this intentional?
For better or worse, the LastIndexOfAny(ROS)
overload is marked with AggressiveInlining
, so this Span
overload will now be more expensive for const values.
At the very least, it's now even more inconsistent with other places (e.g. IndexOfAny
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was intentional in that it became consistent with many of the other Span to ROS forwards though I agree it is (now) inconsistent with IndexOfAny(Span, ROS). I probably overlooked that.
I also believed that the annotation was superfluous in such cases - I apologise for the assumption as I did not instrument that.
Hi @adamsitnik, after your #73768 I am submitting a few changes which I noticed and presumed were not intentionally left out:
SpanHelpers.Char
specialisation toLastIndexOf(Span, ROS)
to matchLastIndexOf(ROS, ROS)
. I did not forward theSpan
version toROS
version to be consistent withIndexOf(Span, ROS)
andIndexOf(ROS, ROS)
, being wary of related discussion in Vectorize {Last}IndexOf{Any} and {Last}IndexOfAnyExcept without code duplication #73768 (comment)LastIndexOfAny(Span, ROS)
toLastIndexOfAny(ROS, ROS)
to be consistent withIndexOfAny
and considering the next changes:byte
cases toLastIndexOfAny
andIndexOfAny
to be consistent with theshort
pathThings I noticed but did not change, deciding they were intentional:
int-
andlong-
size paths on a few variants. I saw a couple removed in that PRIsBitwiseEquatable
vsCanVectorizeAndBenefit
. I saw some tweaks in the last commit of that PRSpanHelpers.IndexOfAnyValueType
has up to 5-value versions butLastIndexOfAnyValueType
has only up to 4-valueSadly I am having a few troubles getting some benchmarks running. I will submit this as a draft until then, if you see anything I am doing obviously wrong I would appreciate the pointer (I am a rookie with the runtime repo).edit: I was using the wrong corerun path. I changed
dotnet run -c Release -f net8.0 -- --coreRun C:\src\artifacts\bin\coreclr\windows.x64.Release\corerun.exe C:\src\runtime\artifacts\bin\coreclr\windows.x64.Release\corerun.exe
to
dotnet run -c Release -- --coreRun C:\src\artifacts\bin\testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\corerun.exe C:\src\runtime\artifacts\bin\testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\corerun.exe