Bucket offline way assignment by area group - #113
Closed
FrogAi wants to merge 3 commits into
Closed
Conversation
This was referenced Aug 9, 2026
Contributor
Author
|
Closing this version. The measured offline-generation speedup is real, but the implementation adds a soundness-critical index with tradeoffs that are not acceptable for a performance-only change: an O(ways x groups) prepass that becomes multi-minute at whole-world scale, slower one- and two-cell selections, and a narrower latent AREA_BOX_DEGREES contract. A future optimization should use direct group lookup or another simpler index and re-establish byte-for-byte parity and end-to-end benefit against current main. The branch and benchmark evidence remain preserved. |
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.
Summary
Motivation
Offline generation currently tests every scanned way against every selected cell. A 20-degree by 20-degree generation chunk contains 6,400 quarter-degree cells, so a 73,135-way input performs 468,064,000 cell-level overlap checks before serialization.
The generator already organizes output into 2-degree directory groups. This change uses those same groups as a conservative candidate index. Ways belonging to one group are checked only against that group's cells. Ways that can reach multiple groups are retained once in a shared list and checked where needed. The final membership decision remains the existing
way.Box.Overlapping(area.OverlapBox(s.Overlap))predicate.Performance
The benchmark used the same normalized Delaware PBF, 20-degree by 20-degree output box, 0.001-degree overlap, filesystem, and pinned Linux/amd64 Go 1.25.1 environment for both revisions. Each result is the median of ten measured process runs after one warm-up per revision, using repeated ABBA ordering.
68813e0)01405ab)The optimization was also applied on top of PR #97's exact area grid and PR #112's empty-cell suppression, then measured with
GenerateEmptyFiles=false:Peak RSS is reported directly rather than inferred from Go allocation and varied more than elapsed time across runs. The benchmark is a chunk-shaped sparse Delaware workload, not a planet-wide density estimate.
Candidate reduction
The same fixture produced this assignment count:
The group-index row is an upper bound because the per-way group scan stops at the second match, so the three multi-group ways terminate early. Including index construction, overlap checks fell from 468,064,000 to at most 12,013,148 for this fixture. Each scanned way contributes at most one retained index, so widespread ways do not create a way-by-group memory cross-product.
Validation
An external comparison harness exercised the final source against base
68813e05b4db68764b7d9dbb73b7998726223c45:0ab87ac90380b4ed2d28cc898f64b4c0b069a68adb0717bb5280ce46b7ccb195.bdfbc23fab9e4afb8475ae73b77e445ef97a5118091da37e5486450a35121022.go test ./...,go vet ./..., andgo build ./...passed on Linux/amd64 with Go 1.25.1.e45faaa1b2fe4cd82160340670962aa44e9ae4bd. That workflow only compiles; it does not rungo test,go vet, orgofmt.01405abb5fe7059d0f8934d31df903c49f84935e. The current head adds two behavior-preserving cleanups on top: the standard librarymathimport is no longer aliased, matchingmaps/way.goandmaps/offline.go, and the merged way index is declared withvarinstead of a discarded zero initializer.The fixture contains 73,135 located ways and has SHA-256
7695b17693df18f2f76a115f4d7f2385034f7b89b8f1d82ba9e2ea088993addc.Compatibility
Merge order with #112 — read before resolving
Merging this PR with #112 conflicts, and the resolution a merge tool suggests silently doubles every way in every tile. #112 moves the way-assignment loop; this PR rewrites it in place.
git merge-treeconfirms git auto-applies the plainfor _, way := range scannedWaysloop outside the conflict region, marking only the deletion as conflicting — so keeping the HEAD side, the obvious choice, leaves both loops.The result compiles, vets, passes
go test, and goes green in CI while writing every way twice. Reproduced concretely: cell10.000000_20.000000_10.250000_20.250000receives[100 100]instead of[100]; a full run measured 151,994 ways with 75,997 duplicate IDs and 2x tile size. The opposite resolution fails loudly with unused locals, so keep-both is the only silently-wrong option, and it is the one a merge tool picks.This is symmetric — not a defect in either PR — and would arise with any pair editing that loop. Whichever lands second must delete the auto-applied plain loop and move this PR's
if bucketed {...} else {...}block up into its place. #97 now merges with zero manual resolution, since thestdmathimport alias that caused its conflict was removed in this PR's second commit.Known limits, deliberately not changed here
maps/generate_offline.goranges every group per way and breaks only on a second match, so 0- and 1-match ways scan all 16,200 groups at whole-world bounds — measured ~163-214 us/way, roughly 18 minutes for a 5M-way extract, with no log line before it (it runs ahead ofFinding Bounds, so it presents as a hang). This is still a large net win over base, which extrapolates to ~11.7 hours, so it is a missed optimization rather than a regression.Box.GroupPos()could answer it in O(1), but that rewrite has a real edge case aroundmath.Floorversus the inclusive edges ofOverlapping, so it is left for a follow-up.canBucketWayis kept even though it is provably dead today. Independent proofs established that noTmpWaythe osmpbf decoder can produce fails it: coordinates are int64-derived so cannot be NaN or Inf, and the min/max fold cannot invert whenlen(Nodes) > 1. It is retained anyway because the proof depends on a third-party decoder's internals and the failure mode if that ever changes is silently dropped roads. It runs once per way during offline generation, not on the device. The separates.Overlapguard is live via--overlapand verified load-bearing atoverlap <= -0.126.AREA_BOX_DEGREEScontract.settings/const.gopermits "1.0 divided by an integer", but the group nesting is only exact for powers of two; 1/3 or 1/5 would leave 396 of 1296 cells poking outside their group and drop ways. Latent today because the configured value is a power of two..github/workflows/build.ymlruns no tests at all. If #99 lands, any of the audit harnesses used here trims into a real test in under 100 lines.