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

Various Hashing Improvements #660

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Various Hashing Improvements #660

wants to merge 4 commits into from

Conversation

stevenewald
Copy link
Contributor

No description provided.

@stevenewald stevenewald marked this pull request as draft January 22, 2024 23:36
@stevenewald stevenewald changed the title HollowOrdinalMapper - resize to next largest doubled prime Various Hashing Improvements Jan 26, 2024
@@ -59,6 +59,20 @@ public class ByteArrayOrdinalMap {

private long[] pointersByOrdinal;

private static int nextLargestPrime(int num) {
int[] precomputedPrimes = new int[]
Copy link
Contributor

Choose a reason for hiding this comment

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

If we pick prime numbers close to these numbers that are also (2^n-1), then we can replace the modulo with simpler bitwise operations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Note the sizable gaps between sequential Mersenne primes (like 127 and 521), which might impact resizing memory efficiency. Considering we're not using modulo during LP, I think the current approach is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Restricting to mersenne primes will result in significant memory overhead -- each mersenne number is ~double the previous, and many of these are not prime, so an increase will often result in a significant step larger than doubling the space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah they seem too few and far between 👍

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.

3 participants