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

Optionally cache CH/MLD edge distances #5197

Closed
wants to merge 5 commits into from

Conversation

danpat
Copy link
Member

@danpat danpat commented Sep 5, 2018

After a long and sordid path, we added annotations=distances to the table plugin back in #4990 and #5019

These PRs implemented on-the-fly distance calculations - the table results are obtained, then the paths are unpacked to calculate the distances.

The upside to this is that it costs nothing in terms of resources (no extra preprocessing, no extra RAM) if you don't use this feature.

The downside is that it's a bit slow. With CH, large matrices that take 1-2 seconds to complete returning just durations can take upwards of 60 seconds to complete when distances are included.

@niemeier-PSI submitted #2764 way back which took the other approach - pre-calculate edge distances, and store them as an additional edge property. This approach is nice and fast, but has the downside that it requires quite a bit more RAM.

Fast-forward to 2018, @TheMarex dropped #4960 on us, which gives us a lot more flexibility with data formats. Suddenly, it's significantly easier to make some things optional, without awful hacks in our file reading code.

This PR is a re-implementation of #2764 but the overhead is optional, and doesn't require compile-time modifications to the OSRM codebase to switch.

Once this is complete, #2764 will be redundant.

** WORK IN PROGRESS - DO NOT MERGE **

Tasklist

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@danpat danpat added this to the 5.20.0 milestone Sep 5, 2018
@danpat danpat self-assigned this Sep 5, 2018
@danpat
Copy link
Member Author

danpat commented Sep 7, 2018

I've implemented cached distance calculations for CH, roughly re-implementing #2764. I haven't completed it for MLD yet, although some work has been done.

Next step is to refactor this to make it optional.

Performance-wise, it works as expected. For a large 500x2000 matrix, the timing is roughly:

Durations only - 1.5 seconds
Durations+distances before this change - 55 seconds
Durations+distances with this change - 1.9 seconds

All the distance_matrix tests pass in CH mode (but not MLD).

@chaupow chaupow force-pushed the danpat_optional_distance_cache branch 2 times, most recently from 99ffc86 to 47509f5 Compare September 7, 2018 17:02
@systemed systemed mentioned this pull request Sep 18, 2018
@chaupow
Copy link
Member

chaupow commented Oct 1, 2018

@danpat I hope you don't mind but as dev is happening over in #5201, I am going to close this PR 🙏

@chaupow chaupow closed this Oct 1, 2018
@danpat danpat deleted the danpat_optional_distance_cache branch October 29, 2018 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants