Fix metric offset overflow for large MLD partitions #6124
Merged
Conversation
This file contains 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
mjjbell
force-pushed
the
mbell/mld_offset_overflow
branch
2 times, most recently
from
746c710
to
56da876
Compare
mjjbell
commented
Sep 13, 2021
| using ValueOffset = std::uint32_t; | ||
| using BoundaryOffset = std::uint32_t; | ||
| using ValueOffset = std::size_t; | ||
| using BoundaryOffset = std::size_t; | ||
| using BoundarySize = std::uint32_t; |
There was a problem hiding this comment.
OSM contains ~800 million ways, so 32 bit should be safe to use for boundary sizes...for now.
mjjbell
commented
Sep 13, 2021
mjjbell
force-pushed
the
mbell/mld_offset_overflow
branch
2 times, most recently
from
de8e9cb
to
d86092c
Compare
Each MLD cell has source and destination nodes. MLD is keeping a |source| x |destination| sized table for various metrics (distances, durations, etc) from each source to all destinations in a cell. It stores all of the values for a metric in one large array, with an offset for each cell to find its values. The offset is currently limited to 32 bit values, which overflows on very large graphs (e.g. Planet OSM). We fix this by changing the offsets to be uint64_t types.
mjjbell
force-pushed
the
mbell/mld_offset_overflow
branch
from
d86092c
to
a85d2e7
Compare
danpat
approved these changes
Sep 21, 2021
|
@mjjbell I think I'm going to make a new release once this is merged - any objections? |
|
@danpat sounds good 👍 I can see I tested the package publishing from Github Actions in https://github.com/mjjbell/osrm-backend/actions/runs/998677972, but I think I must have removed the generated artifacts to not confuse myself in the future. |
|
Proof that it is indeed a breaking change to the data format: #6128 |
This was referenced Sep 21, 2021
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.
Issue
As discovered in #6121
Each MLD cell has source and destination nodes. MLD is keeping a |source| x |destination| sized table
for various metrics (distances, durations, etc) from each source to all destinations in a cell.
It stores all of the values for a metric in one large array, with an offset for each cell to find its values. The offset is currently
limited to 32 bit values, which overflows on very large graphs (e.g. Planet OSM).
We fix this by changing the offsets to be uint64_t types.
Tasklist