Add XxHash128 #77944

xoofx · 2022-11-05T14:57:53Z

Lots of changes in XxHash3 because I had to extract a good chunk of the shared implementation for XXH3 to a shared class XxHashShared.

Performance wise, it is overall under 10% slower compared to C++, but also sometimes 10% faster. On the large item, it is closer to 30% slower, but I noticed that the warmup was only 10% slower, while the actual run was getting slower, not sure if it is a tiered compilation issue.

You will notice that I have introduced constants for all the secrets (e.g DefaultSecretUInt64_0), as they are shared between XXH3-64 and XXH3-128 (before they were inlined and hardcoded).

I have added similar tests to XxHash3, by using the native implementation to generate the expected test results.

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22621.674)
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.100-rc.1.22431.12
  [Host]     : .NET 7.0.0 (7.0.22.42610), X64 RyuJIT AVX2
  DefaultJob : .NET 7.0.0 (7.0.22.42610), X64 RyuJIT AVX2

Method	data	Mean	Error	StdDev	Ratio
XXH3 native	Byte[0]	7.138 ns	0.0139 ns	0.0130 ns	1.00
XXH3 C#	Byte[0]	7.677 ns	0.0121 ns	0.0113 ns	1.08
XXH3 native	Byte[3]	7.726 ns	0.0359 ns	0.0336 ns	1.00
XXH3 C#	Byte[3]	7.592 ns	0.0317 ns	0.0281 ns	0.98
XXH3 native	Byte[8]	7.265 ns	0.0099 ns	0.0087 ns	1.00
XXH3 C#	Byte[8]	7.411 ns	0.0108 ns	0.0095 ns	1.02
XXH3 native	Byte[16]	7.784 ns	0.0282 ns	0.0264 ns	1.00
XXH3 C#	Byte[16]	7.075 ns	0.0270 ns	0.0253 ns	0.91
XXH3 native	Byte[128]	10.483 ns	0.0250 ns	0.0234 ns	1.00
XXH3 C#	Byte[128]	10.671 ns	0.0412 ns	0.0344 ns	1.02
XXH3 native	Byte[240]	21.07 ns	0.043 ns	0.036 ns	1.00
XXH3 C#	Byte[240]	18.94 ns	0.033 ns	0.031 ns	0.90
XXH3 native	Byte[1048576]	27,361.578 ns	48.3501 ns	45.2267 ns	1.00
XXH3 C#	Byte[1048576]	35,092.181 ns	304.4434 ns	284.7766 ns	1.28

cc: @stephentoub, @EgorBo

ghost · 2022-11-05T14:58:06Z

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #77885

Lots of changes in XxHash3 because I had to extract a good chunk of the shared implementation for XXH3 to a shared class XxHashShared.

For large buffers, performance is on par with XxHash3, as it is using the shared implementation.
Compare to native, for intermediate buffers, I haven't checked XxHash3, but the performance is okish, but might require more optimizations here and there. I will try to add some of them as part of this PR If I find some (e.g see for example the 240 bytes buffer that is almost 6 times slower)

You will notice that I have introduced constants for all the secrets (e.g DefaultSecretUInt64_0), as they are shared between XXH3-64 and XXH3-128.

I have added similar tests to XxHash3, by using the native implementation to generate the expected test results.

This PR will have to wait for #77881 to be merged (and will have to be rebased, because XxHash3 file will conflict).

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22621.674)
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.100-rc.1.22431.12
  [Host]     : .NET 7.0.0 (7.0.22.42610), X64 RyuJIT AVX2
  DefaultJob : .NET 7.0.0 (7.0.22.42610), X64 RyuJIT AVX2

Method	data	Mean	Error	StdDev
XXH3Native	Byte[0]	7.137 ns	0.0157 ns	0.0147 ns
XXH3	Byte[0]	12.110 ns	0.0894 ns	0.0836 ns
XXH3Native	Byte[1048576]	27,354.139 ns	92.2719 ns	86.3112 ns
XXH3	Byte[1048576]	31,880.918 ns	33.4602 ns	31.2987 ns
XXH3Native	Byte[128]	10.459 ns	0.0208 ns	0.0184 ns
XXH3	Byte[128]	28.779 ns	0.2224 ns	0.1857 ns
XXH3Native	Byte[16]	7.715 ns	0.0150 ns	0.0140 ns
XXH3	Byte[16]	17.824 ns	0.0928 ns	0.0868 ns
XXH3Native	Byte[240]	21.043 ns	0.0578 ns	0.0512 ns
XXH3	Byte[240]	118.980 ns	0.4931 ns	0.4612 ns
XXH3Native	Byte[3]	7.745 ns	0.0273 ns	0.0255 ns
XXH3	Byte[3]	13.438 ns	0.0067 ns	0.0059 ns
XXH3Native	Byte[8]	7.286 ns	0.0131 ns	0.0122 ns
XXH3	Byte[8]	14.442 ns	0.0680 ns	0.0636 ns

cc: @stephentoub, @EgorBo

Author:	xoofx
Assignees:	-
Labels:	`area-System.Security`
Milestone:	-

# Conflicts: # src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs

xoofx · 2022-11-05T15:42:36Z

Commit 548c2e8 is fixing the performance difference now on part with the C++ version. Should have done before submitting this PR! 😅
I have updated the benchmarks in the PR description.

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHashShared.cs

GrabYourPitchforks · 2022-11-05T16:49:02Z

I've added the NO MERGE label temporarily since this hasn't yet gone through API review. Once that happens, anybody should feel free to remove that label and continue with the standard PR review. :)

(Normal practice is to close PRs where API review hasn't yet taken place, but given that this API is likely to go through as-is I don't think closing this PR is warranted.)

src/libraries/System.IO.Hashing/tests/XxHash128Tests.cs

xoofx · 2022-11-06T09:17:31Z

I ran the tests on a ARM64 and apart the optimized versions that is using vectorized HW intrinsincs, most of the scalar paths are quite slower (from 30% to 2x times slower). I haven't checked the generated code, but the ARM64 JIT is definitely struggling there.

That would be a good playground to optimize things further for ARM64 actually?

BenchmarkDotNet=v0.13.2, OS=Windows 11 (10.0.22621.755)
Snapdragon Compute Platform, 1 CPU, 8 logical and 8 physical cores
.NET SDK=7.0.100-rc.2.22477.23
  [Host]     : .NET 7.0.0 (7.0.22.47203), Arm64 RyuJIT AdvSIMD
  DefaultJob : .NET 7.0.0 (7.0.22.47203), Arm64 RyuJIT AdvSIMD

Method	data	Mean	Error	StdDev	Ratio
XXH3 native	Byte[0]	3.010 ns	0.0077 ns	0.0072 ns	1.00
XXH3 C#	Byte[0]	6.586 ns	0.0124 ns	0.0110 ns	2.19
XXH3 native	Byte[3]	4.215 ns	0.0123 ns	0.0115 ns	1.00
XXH3 C#	Byte[3]	8.335 ns	0.0105 ns	0.0094 ns	1.98
XXH3 native	Byte[8]	4.042 ns	0.0117 ns	0.0104 ns	1.00
XXH3 C#	Byte[8]	5.312 ns	0.0139 ns	0.0130 ns	1.31
XXH3 native	Byte[16]	5.787 ns	0.0107 ns	0.0100 ns	1.00
XXH3 C#	Byte[16]	5.212 ns	0.0131 ns	0.0116 ns	0.90
XXH3 native	Byte[128]	13.538 ns	0.0233 ns	0.0218 ns	1.00
XXH3 C#	Byte[128]	19.959 ns	0.0399 ns	0.0374 ns	1.47
XXH3 native	Byte[240]	28.492 ns	0.0492 ns	0.0460 ns	1.00
XXH3 C#	Byte[240]	36.120 ns	0.0753 ns	0.0704 ns	1.27
XXH3 native	Byte[1048576]	70,504.056 ns	269.1166 ns	251.7318 ns	1.00
XXH3 C#	Byte[1048576]	61,561.207 ns	284.8987 ns	237.9033 ns	0.87

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash128.cs

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHashShared.cs

src/libraries/System.IO.Hashing/tests/XxHash128Tests.cs

src/libraries/System.IO.Hashing/tests/XxHash3Tests.cs

src/libraries/System.IO.Hashing/tests/XxHash64Tests.cs

xoofx · 2022-11-07T17:11:32Z

Thanks for the PR review! I'm on a business travel this week but I will get back to this PR hopefully next week.

stephentoub · 2022-11-08T23:06:01Z

The API was approved. As were the one-shot helpers in #76279. When you swing back around to working on this, can you please add those HashToUInt128 and GetCurrentHashAsUInt128 members as well? Thanks.

Co-authored-by: Stephen Toub <[email protected]>

xoofx · 2022-11-12T14:38:08Z

I'm not sure about how to add the new API to ref. Is it correct to use #if/#endif with NET7_0_OR_GREATER?

    public sealed partial class XxHash128 : System.IO.Hashing.NonCryptographicHashAlgorithm
    {
        public XxHash128() : base (default(int)) { }
        public XxHash128(long seed) : base (default(int)) { }
        public override void Append(System.ReadOnlySpan<byte> source) { }
        protected override void GetCurrentHashCore(System.Span<byte> destination) { }
#if NET7_0_OR_GREATER
        [System.CLSCompliantAttribute(false)] public UInt128 GetCurrentHashAsUInt128() { throw null; }
#endif
        public static byte[] Hash(byte[] source) { throw null; }
        public static byte[] Hash(byte[] source, long seed) { throw null; }
        public static byte[] Hash(System.ReadOnlySpan<byte> source, long seed = (long)0) { throw null; }
        public static int Hash(System.ReadOnlySpan<byte> source, System.Span<byte> destination, long seed = (long)0) { throw null; }
#if NET7_0_OR_GREATER
        [System.CLSCompliantAttribute(false)] public static UInt128 HashToUInt128(ReadOnlySpan<byte> source, long seed = 0) { throw null; }
#endif
        public override void Reset() { }
        public static bool TryHash(System.ReadOnlySpan<byte> source, System.Span<byte> destination, out int bytesWritten, long seed = (long)0) { throw null; }
    }

stephentoub · 2022-11-16T01:42:40Z

I'm not sure about how to add the new API to ref. Is it correct to use #if/#endif with NET7_0_OR_GREATER?

We generally split it across files, e.g.
https://github.com/dotnet/runtime/tree/main/src/libraries/System.Threading.Channels/ref

# Conflicts: # src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs

xoofx · 2022-11-17T18:09:58Z

I have merged latest master and fixed the ref file. (2 pipelines are failing but with a weird error on Azure, but don't think it's coming from this PR)

stephentoub

Thanks.

src/libraries/System.IO.Hashing/ref/System.IO.Hashing.netcoreapp.cs

src/libraries/System.IO.Hashing/tests/XxHash64Tests.cs

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash128.cs

…pp.cs Co-authored-by: Stephen Toub <[email protected]>

…8.cs Co-authored-by: Stephen Toub <[email protected]>

Linq

stephentoub

Thanks!

ViktorHofer · 2022-12-02T12:49:30Z

src/libraries/System.IO.Hashing/ref/System.IO.Hashing.csproj

@@ -8,6 +8,10 @@
    <Compile Include="System.IO.Hashing.cs" />
  </ItemGroup>

+  <ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">


nit: It would have been better to condition on a compatibility check / version check:

<ItemGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0'))">

to not lose the API on net7.0 when $(NetCoreAppCurrent) becomes net8.0. This just started failing in the PR that upgrades to net8.0: https://github.com/dotnet/runtime/pull/78354/checks?check_run_id=9846188314.

I will change this in our PR.

uweigand · 2022-12-02T20:42:02Z

Looks like this commit caused CI failures on the big-endian s390x target:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=100464&view=logs&j=7ea23876-d932-5222-06a0-de72daa3cbf4&t=879dfc24-2817-51ab-29eb-686cb2c04225&l=55

/__w/1/s/.packages/microsoft.dotnet.helix.sdk/8.0.0-beta.22579.2/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(43,5): error : Test System.IO.Hashing.Tests.XxHash128Tests.Hash_Streaming_Expected has failed. Check the Test tab or this console log: https://helix.dot.net/api/2019-06-17/jobs/ce5f39e5-2814-4f1d-88c3-9e58bd385d9f/workitems/System.IO.Hashing.Tests/console [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/8.0.0-beta.22579.2/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(43,5): error : Test System.IO.Hashing.Tests.XxHash128Tests.Hash_OneShot_Expected has failed. Check the Test tab or this console log: https://helix.dot.net/api/2019-06-17/jobs/ce5f39e5-2814-4f1d-88c3-9e58bd385d9f/workitems/System.IO.Hashing.Tests/console [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/8.0.0-beta.22579.2/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(43,5): error : Test System.IO.Hashing.Tests.XxHash3Tests.Hash_Streaming_Expected has failed. Check the Test tab or this console log: https://helix.dot.net/api/2019-06-17/jobs/ce5f39e5-2814-4f1d-88c3-9e58bd385d9f/workitems/System.IO.Hashing.Tests/console [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/8.0.0-beta.22579.2/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(43,5): error : Test System.IO.Hashing.Tests.XxHash3Tests.Hash_OneShot_Expected has failed. Check the Test tab or this console log: https://helix.dot.net/api/2019-06-17/jobs/ce5f39e5-2814-4f1d-88c3-9e58bd385d9f/workitems/System.IO.Hashing.Tests/console [/__w/1/s/src/libraries/sendtohelixhelp.proj]
    0 Warning(s)

I think the endian fixes provided by @stephentoub in #78084 got lost in the move from XXHash3.cs to XXHashShared.cs.

xoofx · 2022-12-03T07:23:52Z

I think the endian fixes provided by @stephentoub in #78084 got lost in the move from XXHash3.cs to XXHashShared.cs.

Ouch, indeed, my apologize for this regression, I pushed the PR #79186 to backport the fix.

Add XxHash128

3b78dd9

ghost added area-System.Security community-contribution Indicates that the PR has been added by a community member labels Nov 5, 2022

xoofx added 2 commits November 5, 2022 16:12

Merge branch 'main' into add-xxhash128

c6bf653

# Conflicts: # src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs

Optimize XxHash128

548c2e8

Missing optimization

4c449c0

teo-tsirpanis reviewed Nov 5, 2022

View reviewed changes

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHashShared.cs Show resolved Hide resolved

GrabYourPitchforks added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 5, 2022

xoofx added 4 commits November 5, 2022 18:06

Fix warnings

5a19745

Optimize the 129 To 240 bytes case

7b916f1

Fix warnings

645dc0e

Add missing namespace for tests

28cf734

vcsjones reviewed Nov 5, 2022

View reviewed changes

src/libraries/System.IO.Hashing/tests/XxHash128Tests.cs Outdated Show resolved Hide resolved

xoofx added 5 commits November 5, 2022 20:45

Split strings in XxHash128 test

76d86cc

Try to fix tests on CI

08f670e

Add XxHash128 to ref

6f722a5

Fix compilation errors on non .NET 7 platforms

5c1c2a7

Fix compilation errors on net472

ce89e5c

build-analysis bot mentioned this pull request Nov 6, 2022

Tracking issue for CI build timeouts #76454

Closed

stephentoub reviewed Nov 7, 2022

View reviewed changes

stephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 8, 2022

xoofx and others added 3 commits November 12, 2022 08:42

Update src/libraries/System.IO.Hashing/tests/XxHash64Tests.cs

dc4a906

Co-authored-by: Stephen Toub <[email protected]>

Update src/libraries/System.IO.Hashing/tests/XxHash3Tests.cs

75fc262

Co-authored-by: Stephen Toub <[email protected]>

Update src/libraries/System.IO.Hashing/tests/XxHash128Tests.cs

ea57cfc

Co-authored-by: Stephen Toub <[email protected]>

Try to fix ref file

fe37c28

xoofx mentioned this pull request Nov 12, 2022

ARM64: Missing combine eor+lsr and duplicate constant reloads #78263

Closed

Fix ref for .NET 7

3513d77

xoofx added 3 commits November 17, 2022 07:39

Split ref files

7c21a47

Merge remote-tracking branch 'origin/main' into add-xxhash128

7fb2234

# Conflicts: # src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash3.cs

Fix ref after merge

5061e3c

build-analysis bot mentioned this pull request Nov 17, 2022

System.IO.Pipes.Tests.NamedPipeTest_Specific.ClientConnectAsync_Cancel_With_InfiniteTimeout failing on Libraries Test Run release coreclr windows x86 Release #69101

Open

jeffhandley added area-System.IO.Hashing and removed area-System.Security labels Nov 22, 2022

stephentoub approved these changes Dec 1, 2022

View reviewed changes

xoofx and others added 8 commits December 1, 2022 19:31

Update src/libraries/System.IO.Hashing/ref/System.IO.Hashing.netcorea…

6a8102f

…pp.cs Co-authored-by: Stephen Toub <[email protected]>

Update src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash12…

63f2d9f

…8.cs Co-authored-by: Stephen Toub <[email protected]>

Remove System.Linq

0599ba4

Linq

Use PascalCast for local const

4505e57

Remove XorShift64 from XxHash128

af4cb41

Remove duplicated Multiply32To64

620ef11

Improve HashLength129To240

74eaced

Reverting System.Linq for XxHash3 and XxHash128

d4a9c9a

build-analysis bot mentioned this pull request Dec 1, 2022

System.IO.Pipes.Tests.NamedPipeTest_CurrentUserOnly_Windows.Connection_UnderDifferentUsers_BehavesAsExpected test failure #79052

Closed

stephentoub approved these changes Dec 2, 2022

View reviewed changes

stephentoub merged commit 8e8ea1b into dotnet:main Dec 2, 2022

ViktorHofer reviewed Dec 2, 2022

View reviewed changes

uweigand mentioned this pull request Dec 2, 2022

Fix NonCryptoHashTestDriver.AssertEqualHashNumber for big endian #79135

Merged

xoofx mentioned this pull request Dec 3, 2022

Fix XxHash3/XxHash128 DeriveSecretFromSeed scalar fallback on big endian #79186

Merged

ghost locked as resolved and limited conversation to collaborators Jan 2, 2023

Add XxHash128 #77944

Add XxHash128 #77944

Uh oh!

Conversation

xoofx commented Nov 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 5, 2022

Uh oh!

xoofx commented Nov 5, 2022

Uh oh!

Uh oh!

GrabYourPitchforks commented Nov 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

xoofx commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xoofx commented Nov 7, 2022

Uh oh!

stephentoub commented Nov 8, 2022

Uh oh!

xoofx commented Nov 12, 2022

Uh oh!

stephentoub commented Nov 16, 2022

Uh oh!

xoofx commented Nov 17, 2022

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

stephentoub Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

uweigand commented Dec 2, 2022

Uh oh!

xoofx commented Dec 3, 2022

Uh oh!

Uh oh!

xoofx commented Nov 5, 2022 •

edited

Loading

GrabYourPitchforks commented Nov 5, 2022 •

edited

Loading

xoofx commented Nov 6, 2022 •

edited

Loading