Repository navigation
Reimplement gridDisk internals with BFS - #1217
Open
isaacbrodsky wants to merge 10 commits into
Open
isaacbrodsky wants to merge 10 commits into
isaacbrodsky wants to merge 10 commits into
Conversation
isaacbrodsky
marked this pull request as ready for review
September 24, 2026 20:14
isaacbrodsky
requested review from
ajfriend,
dfellis,
dmitryzv,
jongbinjung and
nrabinowitz
September 24, 2026 20:14
3 tasks done
justinhwang
added a commit
to uber/h3-go
that referenced
this pull request
Sep 28, 2026
Port the visited-set half of uber/h3#1217 to the pure-Go safe grid disk traversal. The C PR replaces a recursive fallback with a BFS backed by an open-addressing hash set sized at twice the disk; x/h3go was already a queue-based BFS, so the remaining gap was the Go map used to track visited cells. Replace it with a flat slice hash set (zero Cell as the empty marker, linear probing) and use each completed ring as the queue for the next, which drops the separate queue and its growth. Add BenchmarkGridDiskPentagon to paritytest so the pentagon fallback path is measured at k=10..40. benchstat, Go before vs after (Apple M3 Max, count=6): │ old │ new │ │ sec/op │ sec/op vs base │ GridDiskDistancesSafe/impl=go-16 8.905µ ± 6% 3.858µ ± 3% -56.68% (p=0.002 n=6) GridDiskPentagon/k=10/impl=go-16 51.53µ ± 5% 30.36µ ± 1% -41.09% (p=0.002 n=6) GridDiskPentagon/k=20/impl=go-16 223.6µ ± 2% 121.9µ ± 3% -45.47% (p=0.002 n=6) GridDiskPentagon/k=30/impl=go-16 501.3µ ± 2% 273.4µ ± 4% -45.46% (p=0.002 n=6) GridDiskPentagon/k=40/impl=go-16 939.9µ ± 2% 496.2µ ± 2% -47.20% (p=0.002 n=6) geomean 137.0µ 72.02µ -47.45% allocs/op: GridDiskDistancesSafe 20 -> 4; GridDiskPentagon 30/47/65/91 -> 5. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
justinhwang
added a commit
to uber/h3-go
that referenced
this pull request
Sep 28, 2026
Port the visited-set half of uber/h3#1217 to the pure-Go safe grid disk traversal. The C PR replaces a recursive fallback with a BFS backed by an open-addressing hash set sized at twice the disk; x/h3go was already a queue-based BFS, so the remaining gap was the Go map used to track visited cells. Replace it with a flat slice hash set (zero Cell as the empty marker, linear probing) and use each completed ring as the queue for the next, which drops the separate queue and its growth. Add BenchmarkGridDiskPentagon to paritytest so the pentagon fallback path is measured at k=10..40. benchstat, Go before vs after (Apple M3 Max, count=6): │ old │ new │ │ sec/op │ sec/op vs base │ GridDiskDistancesSafe/impl=go-16 8.905µ ± 6% 3.858µ ± 3% -56.68% (p=0.002 n=6) GridDiskPentagon/k=10/impl=go-16 51.53µ ± 5% 30.36µ ± 1% -41.09% (p=0.002 n=6) GridDiskPentagon/k=20/impl=go-16 223.6µ ± 2% 121.9µ ± 3% -45.47% (p=0.002 n=6) GridDiskPentagon/k=30/impl=go-16 501.3µ ± 2% 273.4µ ± 4% -45.46% (p=0.002 n=6) GridDiskPentagon/k=40/impl=go-16 939.9µ ± 2% 496.2µ ± 2% -47.20% (p=0.002 n=6) geomean 137.0µ 72.02µ -47.45% allocs/op: GridDiskDistancesSafe 20 -> 4; GridDiskPentagon 30/47/65/91 -> 5. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
justinhwang
reviewed
Sep 28, 2026
Comment on lines
+146
to
+147
| t_assert(actualAllocCalls == 3, "gridRing called alloc 2 times"); | ||
| t_assert(actualFreeCalls == 3, "gridRing called free 2 times"); |
Contributor
There was a problem hiding this comment.
nit: update comments
Suggested change
| t_assert(actualAllocCalls == 3, "gridRing called alloc 2 times"); | |
| t_assert(actualFreeCalls == 3, "gridRing called free 2 times"); | |
| t_assert(actualAllocCalls == 3, "gridRing called alloc 3 times"); | |
| t_assert(actualFreeCalls == 3, "gridRing called free 3 times"); |
justinhwang
reviewed
Sep 28, 2026
| static GeoPolygon sfGeoPolygon; | ||
|
|
||
| SUITE(h3Memory) { | ||
| TEST(gridDisk) { |
Contributor
There was a problem hiding this comment.
nit: there is no test that gridDiskDistancesSafe or gridDiskDistances with non-NULL distances returns E_MEMORY_ALLOC when the seen calloc fails
justinhwang
reviewed
Sep 28, 2026
| H3Index origin = out[front]; | ||
| if (currK == k) { | ||
| // Does not need to be explored, edge of disk | ||
| continue; |
Contributor
There was a problem hiding this comment.
I think we can skip the rest here
Suggested change
| continue; | |
| break; |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
after
before
Grid disk over pentagons seems to be significantly faster (order of magnitude) with this approach. This avoids the possibility of stack overflow by allocating the memory up front instead. Note that h3o's implementation uses a factor of 2.5 instead of 2 for the hash set size. (Cf. https://github.com/HydroniumLabs/h3o/blob/master/src/grid/iterator.rs#L58)