Skip to content
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

[API Proposal]: XXH3 #75948

Closed
stephentoub opened this issue Sep 21, 2022 · 6 comments · Fixed by #76641
Closed

[API Proposal]: XXH3 #75948

stephentoub opened this issue Sep 21, 2022 · 6 comments · Fixed by #76641
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO.Hashing
Milestone

Comments

@stephentoub
Copy link
Member

Background and motivation

System.IO.Hashing contains implementations of a variety of non-cryptographic hash algorithms, including several from the xxHash family. In particular, it currently includes implementations of XXH32 and XXH64, but not XXH3 nor XXH128. The latter two are vectorizable, with XXH3 clocking in as the fastest of the family, in particular on smaller data.

https://github.com/Cyan4973/xxHash

API Proposal

Exact same shape as the existing XxHash32 and XxHash64, just with a different name.

namespace System.IO.Hashing;

public sealed partial class XxHash3 : NonCryptographicHashAlgorithm
{
    public XxHash3();
    public XxHash3(int seed);

    public static byte[] Hash(byte[] source);
    public static byte[] Hash(byte[] source, int seed);
    public static byte[] Hash(ReadOnlySpan<byte> source, int seed = 0);
    public static int Hash(ReadOnlySpan<byte> source, Span<byte> destination, int seed = 0);
    public static bool TryHash(ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten, int seed = 0);
}

We could also consider adding XxHash128.

API Usage

int bytesWritten = XxHash3.Hash(source, destination);

Alternative Designs

  • We chose to name the XXH32 and XXH64 classes XxHash32 and XxHash64. Feels strange to name the XXH3 class XxHash3, but that would seem to follow the pattern we established. Alternatively, it could just be XXH3.

Risks

No response

@stephentoub stephentoub added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO labels Sep 21, 2022
@stephentoub stephentoub added this to the 8.0.0 milestone Sep 21, 2022
@ghost
Copy link

ghost commented Sep 21, 2022

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

Issue Details

Background and motivation

System.IO.Hashing contains implementations of a variety of non-cryptographic hash algorithms, including several from the xxHash family. In particular, it currently includes implementations of XXH32 and XXH64, but not XXH3 nor XXH128. The latter two are vectorizable, with XXH3 clocking in as the fastest of the family, in particular on smaller data.

https://github.com/Cyan4973/xxHash

API Proposal

Exact same shape as the existing XxHash32 and XxHash64, just with a different name.

namespace System.IO.Hashing;

public sealed partial class XxHash3 : NonCryptographicHashAlgorithm
{
    public XxHash3();
    public XxHash3(int seed);

    public static byte[] Hash(byte[] source);
    public static byte[] Hash(byte[] source, int seed);
    public static byte[] Hash(ReadOnlySpan<byte> source, int seed = 0);
    public static int Hash(ReadOnlySpan<byte> source, Span<byte> destination, int seed = 0);
    public static bool TryHash(ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten, int seed = 0);
}

We could also consider adding XxHash128.

API Usage

int bytesWritten = XxHash3.Hash(source, destination);

Alternative Designs

  • We chose to name the XXH32 and XXH64 classes XxHash32 and XxHash64. Feels strange to name the XXH3 class XxHash3, but that would seem to follow the pattern we established. Alternatively, it could just be XXH3.

Risks

No response

Author: stephentoub
Assignees: -
Labels:

api-suggestion, area-System.IO

Milestone: 8.0.0

@stephentoub
Copy link
Member Author

cc: @bartonjs

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 21, 2022
@ghost
Copy link

ghost commented Sep 26, 2022

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

Issue Details

Background and motivation

System.IO.Hashing contains implementations of a variety of non-cryptographic hash algorithms, including several from the xxHash family. In particular, it currently includes implementations of XXH32 and XXH64, but not XXH3 nor XXH128. The latter two are vectorizable, with XXH3 clocking in as the fastest of the family, in particular on smaller data.

https://github.com/Cyan4973/xxHash

API Proposal

Exact same shape as the existing XxHash32 and XxHash64, just with a different name.

namespace System.IO.Hashing;

public sealed partial class XxHash3 : NonCryptographicHashAlgorithm
{
    public XxHash3();
    public XxHash3(int seed);

    public static byte[] Hash(byte[] source);
    public static byte[] Hash(byte[] source, int seed);
    public static byte[] Hash(ReadOnlySpan<byte> source, int seed = 0);
    public static int Hash(ReadOnlySpan<byte> source, Span<byte> destination, int seed = 0);
    public static bool TryHash(ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten, int seed = 0);
}

We could also consider adding XxHash128.

API Usage

int bytesWritten = XxHash3.Hash(source, destination);

Alternative Designs

  • We chose to name the XXH32 and XXH64 classes XxHash32 and XxHash64. Feels strange to name the XXH3 class XxHash3, but that would seem to follow the pattern we established. Alternatively, it could just be XXH3.

Risks

No response

Author: stephentoub
Assignees: -
Labels:

area-System.Security, api-ready-for-review

Milestone: 8.0.0

@stephentoub stephentoub self-assigned this Sep 27, 2022
@terrajobst
Copy link
Member

terrajobst commented Sep 27, 2022

Video

  • Looks good as proposed
    • We considered also adding XxHash128 but decided to wait until there is a need.
namespace System.IO.Hashing;

public sealed partial class XxHash3 : NonCryptographicHashAlgorithm
{
    public XxHash3();
    public XxHash3(int seed);

    public static byte[] Hash(byte[] source);
    public static byte[] Hash(byte[] source, int seed);
    public static byte[] Hash(ReadOnlySpan<byte> source, int seed = 0);
    public static int Hash(ReadOnlySpan<byte> source, Span<byte> destination, int seed = 0);
    public static bool TryHash(ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten, int seed = 0);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 27, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 4, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 27, 2022
@xoofx
Copy link
Member

xoofx commented Nov 4, 2022

Hey,

We considered also adding XxHash128 but decided to wait until there is a need.

I have a need for 128 bits support, mainly for content addressable storage. I would gladly try to bring support for it.

Do I need to open a separate issue for a XxHash128 API proposal (which would be a copy paste of the XxHash3)?

@krwq
Copy link
Member

krwq commented Nov 4, 2022

@xoofx yes please open a separate issue with API proposal (make sure to describe your scenario)

@ghost ghost locked as resolved and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO.Hashing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants