Add non-cryptographic hash algorithms #53623

bartonjs · 2021-06-02T18:19:54Z

Adds a new package, System.IO.Hashing (netstandard2.0, and net461 to reduce netfx restore graph sizes)
Provides implementations for four hash algorithms
- CRC-32 (the variation used by IEEE 802.3 (Ethernet))
- CRC-64 (the variation used by ECMA-182)
- XxHash32, with optional seed
- XxHash64, with optional seed
The tests are structured so that algorithm tests just need to provide their test vectors and do some boilerplate overrides to get variation testing across the instance methods and static methods.

dotnet-issue-labeler · 2021-06-02T18:19:58Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-06-02T18:20:00Z

Tagging subscribers to this area: @tannergooding, @pgovind
See info in area-owners.md if you want to be subscribed.

Issue Details

Adds a new package, System.IO.Hashing (netstandard2.0, and net461 to reduce netfx restore graph sizes)
Provides implementations for four hash algorithms
- CRC-32 (the variation used by IEEE 802.3 (Ethernet))
- CRC-64 (the variation used by ECMA-182)
- XxHash32, with optional seed
- XxHash64, with optional seed
The tests are structured so that algorithm tests just need to provide their test vectors and do some boilerplate overrides to get variation testing across the instance methods and static methods.

Fixes #24328.

Author:	bartonjs
Assignees:	bartonjs
Labels:	`area-System.Numerics`, `new-api-needs-documentation`
Milestone:	6.0.0

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

stephentoub · 2021-06-02T18:36:28Z

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

+                byte idx = (byte)crc;
+                idx ^= source[i];
+                crc = s_crcLookup[idx] ^ (crc >> 8);
+            }


Would we be able to use any of the hardware intrinsics to speed this up if we had a .NET 6+ build?

+1 to this. If we make this an inbox binary (which I think our partners would really want us to do), it'd make sense to intrinsicify these code paths.

If we make this an inbox binary (which I think our partners would really want us to do)

If I understand correctly, our thoughts for this library is "pure OOB".

Why would the lib need to be inbox to benefit from hardware intrinsics? Doesn't it just need to compile against net6.0 which and could be included in the package?

It only needs to compile against at least 3.0 to get acceleration on x86/x64 and 5.0 for acceleration on ARM64.

In particular, it would be nice if we just put the CRC32C algorithm under: #2036 (which is approved just NYI) and had this API forward to it.
You can then also accelerate plain CRC32 on Arm64, but that requires explicit intrinsics usage outside an xplat helper method.

Well, but those are CRC32C, not CRC32. If there's a sensible way to get from CRC32C to CRC32, then sure.

For regular CRC32 (that is the variant where 0x04C11DB7 is the polynomial) its only available to accelerate on ARM64 (at least via a direct instruction, although technically also for Arm32, we just don't have intrinsic support here).

For CRC32-C (that is the variant where 0x1EDC6F41 is the polynomial) its available to x86/x64 and ARM64.

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash32.State.cs

src/libraries/System.IO.Hashing/pkg/System.IO.Hashing.pkgproj

GrabYourPitchforks · 2021-06-07T21:16:50Z

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

+    /// </summary>
+    /// <remarks>
+    ///   <para>
+    ///     This implementation emits the answer in the Little Endian byte order so that


Not necessary here, but I wonder if our code sample docs should show using BinaryPrimitives.ReadInt32LittleEndian to get this data back out.

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash64.State.cs

GrabYourPitchforks · 2021-06-08T00:39:36Z

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

+namespace System.IO.Hashing
+{
+    /// <summary>
+    ///   Provides an implementation of the XxHash32 algorithm.


If we provide a code sample showing using this and extracting a 32-bit hash, our code sample should demonstrate using BinaryPrimitives.ReadUInt32BigEndian to convert the byte array back to an integer. In particular, devs may be drawn to Convert.ToInt32, which will not provide the correct answer. This code sample doesn't need to be in the devdoc, but I do think it's a potential pit of failure if we don't show an example.

Ref: https://github.com/Cyan4973/xxHash/blob/f9155bd4c57e2270a4ffbb176485e5d713de1c9b/doc/xxhash_spec.md#step-7-output

GrabYourPitchforks · 2021-06-08T00:40:48Z

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

+namespace System.IO.Hashing
+{
+    /// <summary>
+    ///   Provides an implementation of the XxHash64 algorithm.


If we provide a code sample showing using this and extracting a 64-bit hash, our code sample should demonstrate using BinaryPrimitives.ReadUInt64BigEndian to convert the byte array back to an integer. In particular, devs may be drawn to Convert.ToInt64, which will not provide the correct answer. This code sample doesn't need to be in the devdoc, but I do think it's a potential pit of failure if we don't show an example.

Ref: https://github.com/Cyan4973/xxHash/blob/f9155bd4c57e2270a4ffbb176485e5d713de1c9b/doc/xxhash_spec.md#step-7-output-1

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

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash64.State.cs

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

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

src/libraries/pkg/baseline/packageIndex.json

src/libraries/System.IO.Hashing/Directory.Build.props

GrabYourPitchforks · 2021-06-08T21:42:11Z

Code LGTM. Approved pending whatever fixups are necessary to make packaging work. :)

ViktorHofer · 2021-06-08T21:50:21Z

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

+    <TargetFrameworks>net6.0;netstandard2.0;net461</TargetFrameworks>
+    <IsPackable>true</IsPackable>
+    <!-- TODO: Remove when the package ships with .NET 6. -->
+    <DisablePackageBaselineValidation>true</DisablePackageBaselineValidation>


It's actually the inverse. Sorry...

Suggested change

<DisablePackageBaselineValidation>true</DisablePackageBaselineValidation>

<EnablePackageBaselineValidation>false</EnablePackageBaselineValidation>

ViktorHofer · 2021-06-08T21:51:55Z

Looks like you are missing the reference project, in case you want/need it. I don't it's strictly required. cc @ericstj

ViktorHofer

Aside from the ref project question, LGTM. Thanks for addressing the packaging feedback.

bartonjs · 2021-06-08T21:53:50Z

Looks like you are missing the reference project, in case you want/need it. I don't it's strictly required

I copied the structure from System.Formats.Asn1, where I understood it to be "we still generate the .cs file for API evolution/observation purposes, and we still build it to align the src and ref, but we don't need to ship it because we don't do versioned API"... or something like that.

eerhardt · 2021-06-16T16:34:16Z

@bartonjs - was there a reason why this didn't include a \ref\System.IO.Hashing.csproj file? I don't see a "ref" project file in:

https://github.com/dotnet/runtime/tree/main/src/libraries/System.IO.Hashing/ref

Oh, I see the response above. I don't totally understand it, but I guess you addressed the question above.

UPDATE: @ViktorHofer answered all my questions offline.

bartonjs added 9 commits May 28, 2021 14:39

Add CRC-32 and the basic conformance tests

2733ae2

Add CRC-64(-ECMA)

4c0e824

Change test empty hash initializer style

e7f1b82

Add XxHash32

f959a16

Add XxHash64

d56e84e

Add support for seeded hashing to XxHashes

9f34439

Add some missing override apidocs

11ffe57

Some csproj cleanup

ba892cd

Add more variation to the Commonly Used Types

372c796

bartonjs added this to the 6.0.0 milestone Jun 2, 2021

bartonjs self-assigned this Jun 2, 2021

dotnet-issue-labeler bot added area-System.Numerics new-api-needs-documentation labels Jun 2, 2021

stephentoub reviewed Jun 2, 2021

View reviewed changes

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

stephentoub reviewed Jun 2, 2021

View reviewed changes

src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHash32.State.cs Outdated Show resolved Hide resolved

bartonjs added 4 commits June 2, 2021 17:01

Add missing packageIndex data

af9117a

Remove unnecessary unchecked keywords

7379fde

Improve some const folding

f28576e

Make the XxHash64 ctors cascade as expected

5227582

bartonjs mentioned this pull request Jun 3, 2021

Hmac one-shot #53487

Merged

GrabYourPitchforks reviewed Jun 7, 2021

View reviewed changes

src/libraries/System.IO.Hashing/pkg/System.IO.Hashing.pkgproj Outdated Show resolved Hide resolved

GrabYourPitchforks reviewed Jun 8, 2021

View reviewed changes

ViktorHofer reviewed Jun 8, 2021

View reviewed changes

src/libraries/System.IO.Hashing/src/System.IO.Hashing.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Jun 8, 2021

View reviewed changes

src/libraries/System.IO.Hashing/src/System.IO.Hashing.csproj Outdated Show resolved Hide resolved

ViktorHofer reviewed Jun 8, 2021

View reviewed changes

src/libraries/pkg/baseline/packageIndex.json Outdated Show resolved Hide resolved

ViktorHofer reviewed Jun 8, 2021

View reviewed changes

src/libraries/System.IO.Hashing/Directory.Build.props Outdated Show resolved Hide resolved

Respond to feedback

cbef585

bartonjs added 3 commits June 8, 2021 13:22

Fix bad logic for XxHash of 2G..4G(+15 bytes)

74b494f

Remove pkgproj

81e82ef

Add net6.0 compile target

ddd2c83

GrabYourPitchforks approved these changes Jun 8, 2021

View reviewed changes

ViktorHofer reviewed Jun 8, 2021

View reviewed changes

ViktorHofer approved these changes Jun 8, 2021

View reviewed changes

Spell EnablePackageBaselineValidation correctly

504154e

bartonjs merged commit 3e6f2ff into dotnet:main Jun 9, 2021

LeaFrock mentioned this pull request Jun 9, 2021

Can more non-cryptographic hash algorithms be added into .NET BCL? #43131

Open

BruceForstall mentioned this pull request Jun 12, 2021

Assertion failed '((emitThisGCrefRegs | emitThisByrefRegs) & regMask) && (ins == INS_inc || ins == INS_dec || ins == INS_inc_l || ins == INS_dec_l)' #54007

Closed

bartonjs deleted the non_crypto_hashing branch June 17, 2021 16:20

ghost locked as resolved and limited conversation to collaborators Jul 17, 2021

bartonjs removed their assignment Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add non-cryptographic hash algorithms #53623

Add non-cryptographic hash algorithms #53623

bartonjs commented Jun 2, 2021

dotnet-issue-labeler bot commented Jun 2, 2021

ghost commented Jun 2, 2021

stephentoub Jun 2, 2021

GrabYourPitchforks Jun 7, 2021

bartonjs Jun 8, 2021

ViktorHofer Jun 8, 2021

tannergooding Jun 16, 2021

bartonjs Jun 17, 2021

tannergooding Jun 17, 2021

GrabYourPitchforks Jun 7, 2021

GrabYourPitchforks Jun 8, 2021

GrabYourPitchforks Jun 8, 2021

GrabYourPitchforks commented Jun 8, 2021

ViktorHofer Jun 8, 2021

ViktorHofer commented Jun 8, 2021

ViktorHofer left a comment

bartonjs commented Jun 8, 2021

eerhardt commented Jun 16, 2021 •

edited

Loading

	<DisablePackageBaselineValidation>true</DisablePackageBaselineValidation>
	<EnablePackageBaselineValidation>false</EnablePackageBaselineValidation>

Add non-cryptographic hash algorithms #53623

Add non-cryptographic hash algorithms #53623

Conversation

bartonjs commented Jun 2, 2021

dotnet-issue-labeler bot commented Jun 2, 2021

ghost commented Jun 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks commented Jun 8, 2021

Choose a reason for hiding this comment

ViktorHofer commented Jun 8, 2021

ViktorHofer left a comment

Choose a reason for hiding this comment

bartonjs commented Jun 8, 2021

eerhardt commented Jun 16, 2021 • edited Loading

eerhardt commented Jun 16, 2021 •

edited

Loading