Repository navigation
perf: expand geohash sets on packed integers instead of strings - #32
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
There was a problem hiding this comment.
Pull request overview
This PR improves the performance of geohash set expansion by switching the expansion walk from String geohashes (via geohash::neighbors) to a packed u64 representation with an integer-based neighbor walk, then rendering back to base32 only at the end. It also removes the prior boundary pre-pass so zero-hop expansions can return quickly after validation.
Changes:
- Added
ghbits::packandghbits::neighborsto support packed (u64) geohash parsing and neighbor enumeration with pole clamping. - Reworked
expand_geohash_setto expand rings usingFxHashSet<u64>and integer neighbor traversal, converting back toHashSet<String>only once at the end. - Added unit tests covering pack/unpack roundtrips, invalid-character rejection, neighbor behavior (including pole clamping), and expansion correctness vs a reference implementation.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pub fn pack(hash: &str) -> Option<u64> { | ||
| let mut v = 0u64; | ||
| for c in hash.bytes() { | ||
| let idx = BASE32.iter().position(|&b| b == c)?; | ||
| v = (v << 5) | idx as u64; | ||
| } | ||
| Some(v) | ||
| } |
There was a problem hiding this comment.
Agreed, fixed in 2257266. pack now rejects anything outside 1..=MAX_PRECISION up front, with test_ghbits_pack_rejects_out_of_range_lengths covering the empty and 13-character cases.
| Ok(all | ||
| .into_iter() | ||
| .map(|cell| ghbits::unpack(cell, precision)) | ||
| .collect()) |
There was a problem hiding this comment.
Done in 2257266 — the output set is now built with HashSet::with_capacity(all.len()).
65ce366 to
2257266
Compare
2257266 to
dc47d92
Compare
f924e36 to
394db72
Compare
dc47d92 to
237ffcd
Compare
expand_geohash_set walked the grid through geohash::neighbors, which
allocates eight Strings per cell, and kept cells in a HashSet<String> hashed
with SipHash. The allocation churn dominated: the actual work is integer
arithmetic on a grid.
Pack each hash into its u64 form and walk with ghbits::neighbors, keeping
cells in an FxHashSet<u64> and rendering back to base32 once at the end.
Measured end to end from Python on whitehorse p6 (24,571 input cells),
output identical in every case:
expansion_m before after
0 m 15.1 ms 4.3 ms 3.5x
700 m 16.8 ms 6.0 ms 2.8x
3000 m 19.1 ms 6.4 ms 3.0x
12000 m 31.4 ms 8.6 ms 3.7x
The zero-hop case also dropped a wasted pass. The old code scanned every
input cell for boundary membership before looking at n_hops, so an
expansion of 0 m paid the full O(N*8) neighbour scan to return its own
input. Input hashes are still validated in that case — packing does it — so
malformed input is rejected as before.
Ring expansion no longer needs that boundary pre-pass at all: cells already
in the set are never re-added, so interior cells fall out of the frontier
after the first pass on their own.
Two behaviour changes worth calling out:
- Latitude now clamps at the poles instead of wrapping. geohash::neighbors
reports eight neighbours for a top-row cell such as "zzzzzzzzzzzz", three
of which have wrapped over the pole to the opposite edge of the grid, so
expanding a polar geography used to jump to the other hemisphere. Covered
by test_ghbits_neighbors_clamp_at_poles and
test_expand_at_the_pole_does_not_cross_over.
- A set mixing precisions is now an error rather than each cell expanding on
its own grid. Every caller in this crate already validated uniform
precision before calling in.
Squashed with:
- fix: reject out-of-range hash lengths in ghbits::pack
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
237ffcd to
824c376
Compare
ed57150 to
c9b999e
Compare
Problem
expand_geohash_setwalked the grid throughgeohash::neighbors, which allocates eightStrings per cell, and kept cells in aHashSet<String>hashed with SipHash. The allocation churn dominated — the actual work is integer arithmetic on a grid.Separately, the zero-hop case paid a full O(N·8) boundary pre-pass before ever looking at
n_hops, just to return its own input.Fix
Pack each hash into its
u64form and walk withghbits::neighbors, keeping cells in anFxHashSet<u64>and rendering back to base32 once at the end. Ring expansion no longer needs the boundary pre-pass at all: cells already in the set are never re-added, so interior cells fall out of the frontier on their own after the first pass.Input hashes are still validated when
n_hops == 0— packing does it — so malformed input is rejected as before.Benchmarks
End to end from Python, whitehorse p6 (24,571 input cells), output identical in every case:
expansion_mgeohash::neighborsreports eight neighbours for a top-row cell such aszzzzzzzzzzzz, three of which have wrapped over the pole to the opposite edge of the grid — so expanding a polar geography used to jump to the other hemisphere. Covered bytest_ghbits_neighbors_clamp_at_polesandtest_expand_at_the_pole_does_not_cross_over.