-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add XxHash3 to System.IO.Hashing #76641
Conversation
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsFixes #75948 [Params(2, 4, 8, 16, 32, 64, 128, 1024 * 1024)]
public int Count { get; set; }
private byte[] _output = new byte[100];
private byte[] _input;
[GlobalSetup]
public void Setup() => _input = RandomNumberGenerator.GetBytes(Count);
[Benchmark(Baseline = true)] public int XXH32_Hash() => XxHash32.Hash(_input, _output);
[Benchmark] public int XXH64_Hash() => XxHash64.Hash(_input, _output);
[Benchmark] public int XXH3_Hash() => XxHash3.Hash(_input, _output);
[Benchmark] public int Crc32_Hash() => Crc32.Hash(_input, _output);
[Benchmark] public int Crc64_Hash() => Crc64.Hash(_input, _output);
|
Vector128<uint> sourceVec = Vector128.LoadUnsafe(ref sourceRef); | ||
Vector128<uint> sourceKey = sourceVec ^ Vector128.LoadUnsafe(ref keyRef); | ||
|
||
// TODO: Figure out how to unwind this shuffle and just use Vector128.Multiply |
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.
@tannergooding, per our conversation yesterday, I was able to get rid of the other direct usage of Sse2.Multiply with a shuffle done before it, but I couldn't quite wrap my mind around getting rid of this one. If it's obvious to you how, please let me know :)
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
Vector256<ulong> sum = accVec + sourceSwap.AsUInt64(); | ||
Vector256<ulong> product = Avx2.IsSupported ? | ||
Avx2.Multiply(sourceKey, sourceKeyLow) : | ||
(sourceKey & Vector256.Create(~0u, 0u, ~0u, 0u, ~0u, 0u, ~0u, 0u)).AsUInt64() * (sourceKeyLow & Vector256.Create(~0u, 0u, ~0u, 0u, ~0u, 0u, ~0u, 0u)).AsUInt64(); |
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.
@tannergooding is operator *
for Vector128<ulong>
supported on ARM64? judging by the codegen it is not (fallbacks to software impl)
(my comment points to Vector256 but I meant Vector128)
Just to point something out, I realize this is probably an upstream issue in Code which I will file an issue for, but I am unsure if or how that will be addressable. |
Thanks. I'm not sure what to do about that, other than a) reduce the number of tests (I copied these from an internal set of tests from another XXH3 implementation) or b) separate the tests out into a data file that the test then reads from, which feels a bit arbitrary. |
Filed an issue at microsoft/vscode#163434. Largely the problem seems to be lines 3870-3902. When I get rid of the extremely long lines, things go back to being snappy. When I disable syntax highlighting it works too, so it appears to be part of the C# language syntax highlighting. I guess if there were anything to do in this pull request it would be to manually wrap those very long base64 strings from "AAAAAAAA" to "AAAA" + <newline> "AAAAA" so that the lines are < 10,000 characters long. That seems to work for me. |
Thanks. Done. Better? |
Much! Thank you! |
Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones Issue DetailsFixes #75948 [Params(2, 4, 8, 16, 32, 64, 128, 1024 * 1024)]
public int Count { get; set; }
private byte[] _output = new byte[100];
private byte[] _input;
[GlobalSetup]
public void Setup() => _input = RandomNumberGenerator.GetBytes(Count);
[Benchmark(Baseline = true)] public int XXH32_Hash() => XxHash32.Hash(_input, _output);
[Benchmark] public int XXH64_Hash() => XxHash64.Hash(_input, _output);
[Benchmark] public int XXH3_Hash() => XxHash3.Hash(_input, _output);
[Benchmark] public int Crc32_Hash() => Crc32.Hash(_input, _output);
[Benchmark] public int Crc64_Hash() => Crc64.Hash(_input, _output);
|
@bartonjs, any other feedback? |
Well, at a high level, I'd love to get rid of all of the pointers; though maybe the use of Otherwise it was a bit intense for my low-coffee-nated brain this morning and I didn't remember to look at it again today; I'll try to get to it tomorrow afternoon. |
That's part of it. The other part of it is that it's a whole lot faster with the pointers; the way I initially had it, just indexing into spans, the bounds check costs were measurable. They could be eliminated by the JIT if enough things were aggressively inlined so that it could see earlier length checks, but that then resulted in significant size bloat as well as hitting up against inlining limits. I think the use of pointers here is reasonable. It's localized and it matches the reference implementation.
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.
I've made some progress, will have to finish tomorrow.
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs
Outdated
Show resolved
Hide resolved
Thanks for the helpful review, @bartonjs. I addressed all your feedback. |
Fixes #75948