-
Notifications
You must be signed in to change notification settings - Fork 1k
[Optimize
]: Murmur128
is a noncryptographic hash aglorithm
#3919
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
[Optimize
]: Murmur128
is a noncryptographic hash aglorithm
#3919
Conversation
Fix
&Optimize
]: Murmur128
is a noncryptographic hash aglorithmOptimize
]: Murmur128
is a noncryptographic hash aglorithm
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.
Pull Request Overview
This PR implements a noncryptographic version of the Murmur128 hash algorithm and optimizes its performance while updating the associated tests and helper methods.
- Implements Murmur128 as a noncryptographic hash algorithm by deriving from NonCryptographicHashAlgorithm.
- Refactors the hash calculation to use a new Append/ComputeHash pattern with improved tail handling.
- Updates tests and helper extensions to reflect the new API and improved performance.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
tests/Neo.UnitTests/Cryptography/UT_Murmur128.cs | Test adjustments to use the new ComputeHash method and HashSizeInBits. |
src/Neo/Cryptography/Murmur128.cs | Refactor of the Murmur128 implementation to optimize performance and tail processing. |
src/Neo/Cryptography/Helper.cs | Update Murmur128 extension methods to use ReadOnlySpan-based implementation. |
benchmarks/Neo.Benchmarks/Benchmarks.Hash.cs | Introduction of a benchmark for the Murmur128 performance. |
for (; source.Length >= 16; source = source[16..]) | ||
{ | ||
Mix(source); |
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.
[nitpick] Consider refactoring this loop to use an index-based iteration instead of repeatedly slicing the source span. This may help reduce overhead in performance-critical paths.
Copilot uses AI. Check for mistakes.
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.
@Wi1l-B0t Copilot is right, avoiding coy the array and using index it will be faster
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.
@Wi1l-B0t Copilot is right, avoiding coy the array and using index it will be faster
The source
is a span. Slice is fast.
Co-authored-by: Copilot <[email protected]>
Description
Same as the murmur32, murmur128 is a noncryptographic hash aglorithm.
So it shouldn't be implemented as a cryptographic hash (This would be misleading.):
This PR:
A simple benchmark:
Before:
After:
Fixes # (issue)
Type of change
Checklist: