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

Rewrite double hashing algo again, to fix cycle of length size issue #7

Merged
merged 3 commits into from
Dec 13, 2021

Conversation

SimonWoolf
Copy link
Contributor

Enhanced double hashing stops cycles of length less than size in the case where size is coprime with the second hash. But you still get cycles of length size. So if we reach there and haven't finished, append a prime to the input and rehash.

Also fix poorly implemented cubic term: interpreted (n^3 - n)/6 mod m wrongly (division should have been mult inverse under mod m, not integer floor division). Reimplemented correctly.

I think this should now be both efficient and resilient against pathological inputs.

kblauhut and others added 2 commits December 1, 2021 13:49
Enhanced double hashing stops cycles of length less than `size` in the
case where size is coprime with the second hash. But you still get
cycles of length `size`.  So if we reach there and haven't finished,
append a prime to the input and rehash.

Also fix poorly implemented cubic term: interpreted (n^3 - n)/6 mod m
wrongly (division should have been mult inverse under mod m, not integer
floor division). Reimplemented correctly.
Copy link
Owner

@kblauhut kblauhut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its quite confusing how the paper was written. I was also assuming (n^3 - n)/6 mod m to be the equation to be used as a replacement for double hashing.

Cant the doubleHashing function be removed for good now though?

@SimonWoolf
Copy link
Contributor Author

SimonWoolf commented Dec 13, 2021

Cant the doubleHashing function be removed for good now though?

Yes - sorry, I'd done that on my main branch but not on the xxh version, now done

@kblauhut kblauhut changed the base branch from main to dev December 13, 2021 12:41
@kblauhut kblauhut merged commit af7fcdf into kblauhut:dev Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants