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

[WIP] Fast isValidCell #496

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from
Draft
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions src/h3lib/lib/h3Index.c
Original file line number Diff line number Diff line change
Expand Up @@ -344,18 +344,31 @@ int _isValidCell_const(const H3Index h) {
// Pentagon cells start with a sequence of 0's (CENTER_DIGIT's).
// The first nonzero digit can't be a 1 (i.e., "deleted subsequence",
// PENTAGON_SKIPPED_DIGIT, or K_AXES_DIGIT).
// Test for pentagon base cell first to avoid this loop if possible.
if (isBaseCellPentagonArr[bc]) {
H3Index g = h << 19;
H3Index g = h;
g <<= 19;
g >>= 19; // at this point, g < 2^45 - 1
Comment on lines +349 to +350
Copy link
Collaborator

@dfellis dfellis Feb 15, 2022

Choose a reason for hiding this comment

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

Why not replace this with a simple & mask that has 0s for the top 19 bits and 1s for the lower 45 bits? That should be much faster than shifting up and down 19 times each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, agreed!


for (int r = 1; r <= res; r++) {
uint64_t d = GT(g, 3);
g <<= 3;
if (g == 0) return true; // all zeros (res 15 pentagon)

if (d == 0) continue;
if (d == 1) return false;
if (d >= 2) break;
}
// Converting g to a double essentially puts log2(g) into the
// exponent bits of f (with an offset).
double f = g;

// reinterpret the double bits as uint64_t
g = *(uint64_t *)&f;
Comment on lines +360 to +361
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unclear if this is safe - it may be necessary to use a different implementation on a platform like ARM (we can probably test this on Raspberry Pi). We should check if UBSan complains about this.


// Take the 11 exponent bits and the 1 sign bit.
// The sign bit is guaranteed to be 0 in our case.
g = GT(g, 12);

// Subtract the exponent bias.
g -= 1023;

// g now holds the index of (its previous) first nonzero bit.
// The first nonzero digit is a 1 (and thus invalid) if the
// first nonzero bit's position is divisible by 3.
if (g % 3 == 0) return false;
}

// If no flaws were identified above, then the index is a valid H3 cell.
Expand Down