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

Remove distances from CH shortcuts #1719

Merged
merged 5 commits into from Sep 10, 2019

Conversation

@easbar
Copy link
Collaborator

easbar commented Sep 10, 2019

No longer store distances for CH shortcuts, they were not really used anyway.
This reduces memory consumption per shortcut by four bytes (now 9*4=36 bytes for node-based and 11*4=44 bytes for edge-based CH). To retrieve the distance of a shortcut one can use ShortcutUnpacker and sum all the distances of the underlying real edges (this is the way we did it for path extraction already).

Fixes #1690.

easbar added 3 commits Sep 7, 2019
@easbar easbar added the improvement label Sep 10, 2019
@easbar easbar added this to the 0.13 milestone Sep 10, 2019
Copy link
Member

karussell left a comment

This looks really clean. Nice!!

I can imagine that this makes preparation also a bit faster for German-sized installations or is the effect too small?

return ((CHEdgeIteratorState) edge).getWeight();
} else {
return weighting.calcWeight(edge, false, EdgeIterator.NO_EDGE);
}

This comment has been minimized.

Copy link
@karussell

karussell Sep 10, 2019

Member

Why are there two cases now?

This comment has been minimized.

Copy link
@easbar

easbar Sep 10, 2019

Author Collaborator

ah this is just a helper method in this test, only for convenience. previously I used weighting.calcWeight but since weighting is a shortest weighting here this is calling getDistance() (which causes an exception for shortcuts). In production code this is a bit different because we normally would use PreparationWeighting (but that does not have the instanceof check...).

This comment has been minimized.

Copy link
@karussell
@easbar

This comment has been minimized.

Copy link
Collaborator Author

easbar commented Sep 10, 2019

I can imagine that this makes preparation also a bit faster for German-sized installations or is the effect too small?

Will try (its almost certainly not slower, but I am interested how such a change affects performance as well).

@easbar easbar merged commit 2106041 into master Sep 10, 2019
5 checks passed
5 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk - pom.xml (karussell) No new issues
Details
security/snyk - web/package.json (karussell) No manifest changes detected
@easbar easbar deleted the remove_dists_from_shortcuts branch Sep 10, 2019
@easbar

This comment has been minimized.

Copy link
Collaborator Author

easbar commented Sep 10, 2019

This and this commit seem to have improved the (node-based) CH preparation time by about 10-15% (tried this for Germany only)

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Sep 10, 2019

Wow, that is a huge number for already highly tuned stuff. Nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.