-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Vectorize ProbabilisticMap.IndexOfAny #80963
Conversation
Tagging subscribers to this area: @dotnet/area-system-memory Issue Details
Benchmark sourcepublic class ReplaceLineEndingsBenchmark
{
private string _input;
[Params(0.0, 0.05, 0.1, 0.2, 1.0)]
public double NewLineFrequency;
[Params(10_000)]
public int Length;
[GlobalSetup]
public void Setup()
{
char[] input = new char[Length];
var rng = new Random(42);
for (int i = 0; i < input.Length; i++)
{
if (rng.NextDouble() < NewLineFrequency)
{
input[i] = '\n';
}
else
{
char c;
do
{
c = (char)rng.Next(0, 65536);
}
while ("\r\n\f\u0085\u2028\u2029".Contains(c));
input[i] = c;
}
}
_input = new string(input);
}
[Benchmark]
public string ReplaceLineEndings() => _input.ReplaceLineEndings();
} If we care, we could also do this for I don't know if it's possible to do something similar efficiently on ARM given that the bloom filter in this case is 256-bit.
|
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Private.CoreLib/src/System/IndexOfAnyValues/ProbabilisticMap.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IndexOfAnyValues/ProbabilisticMap.cs
Outdated
Show resolved
Hide resolved
f96031d
to
9af50f6
Compare
Added |
9af50f6
to
9c67c08
Compare
9c67c08
to
399770b
Compare
399770b
to
8761d76
Compare
@radekdoulik we should add PackedSimd versions of the Unsafe* functions wasm supports to ease handling all the calling callers. I'm not sure we have a great pattern to simplify the *.IsHardwareAccelerated paths that works for all the cases yet but that is worth considering too. |
Any thoughts on this @dotnet/area-system-memory? |
src/libraries/System.Private.CoreLib/src/System/IndexOfAnyValues/ProbabilisticMap.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IndexOfAnyValues/ProbabilisticMap.cs
Outdated
Show resolved
Hide resolved
Vector128<byte> highNibble = Sse2.IsSupported | ||
? Sse2.ShiftRightLogical(values.AsInt32(), VectorizedIndexShift).AsByte() & Vector128.Create((byte)15) | ||
: AdvSimd.ShiftRightLogical(values, VectorizedIndexShift); |
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.
These are different because shifting via byte
isn't accelerated on xarch, right?
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.
Yes. We do the same thing in IndexOfAnyAsciiSearcher as well, where that also feeds into a shuffle
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.
👍, probably worth opening an issue so an efficient implementation can be done and we can avoid the separate paths here.
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.
src/libraries/System.Private.CoreLib/src/System/IndexOfAnyValues/ProbabilisticMap.cs
Outdated
Show resolved
Hide resolved
Anything else that should be changed here, or is this one good to merge @tannergooding? |
@tannergooding can this be merged? (I'm trying to avoid more merge conflicts as this was already rebased a fair number of times) |
Another merge conflict popped up. Would be good to see some more benchmark numbers to better display where the cutoff is for the index ratio. Likely also needs a secondary review from someone like @stephentoub given the code its touching. |
8482f56
to
a753250
Compare
src/libraries/System.Private.CoreLib/src/System/IndexOfAnyValues/ProbabilisticMap.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector128.cs
Show resolved
Hide resolved
Thanks! |
AVX2
ARM64
Benchmark source
If we care, we could also do this for
LastIndexOfAny
.#80297 could be interesting for ARM if we could do a 256-bit lookup instead of blending together smaller lookups.