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

Speed up generation of step geometries when there are lots of legs #4936

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

danpat
Copy link
Member

@danpat danpat commented Mar 6, 2018

Issue

While profiling the match plugin, I noticed that performance was about 10x worse when returning step geometries in GeoJSON format compared to returning polyline or polyline6 geometries.

A little bit of digging revealed that we were resizing the step_geometries vector for every leg in the route. For long map-matches, there are lots and lots of legs.

Combine this with the fact that util::json::Value is quite expensive to copy (we should probably look at that separately), it looks like if you do steps=true&geometries=geojson on any match request, the response time is completely dominated by calls to .reserve() at the link above.

This change simply pre-allocates the whole vector before looping over all the legs, avoiding the repeated .reserve() calls and all the copies of the data within the step_geometries vector.

There will be little to no performance change on requests with only 1 leg (i.e. simple /route requests).

For the 1200 coordinate /match request I was testing, this simple change reduces the query time from 7854ms down to 560ms.

Tasklist

Copy link
Contributor

@oxidase oxidase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7.8s -> 560ms is a great improvement!

Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow great find! Do you think we can pull this in a bug fix release for 5.16? This would really help with my waypoints benchmarks.

@TheMarex TheMarex added this to the 5.16.1 milestone Mar 6, 2018
@danpat
Copy link
Member Author

danpat commented Mar 7, 2018

@TheMarex yeah, I think this is an easy candidate for 5.16.2, it's a pretty safe change.

Note that the preconditions for the problem are pretty limited:

  • /route with steps=true&geometries=geojson and lots of waypoints creating lots of legs
  • /match with steps=true&geometries=geojson - I think steps=true is probably rare here.

The only time that steps=true currently makes sense with /match is if waypoints=0,n is also used, and that causes there to only be a single leg, which also avoids the problem.

So, yeah, worth putting in 5.16.2, and some scenarios will get a big boost, but unfortunately, I don't think this is our most common query parameter combination :-(

@TheMarex TheMarex merged commit a4ee2cc into master Mar 8, 2018
@TheMarex TheMarex modified the milestones: 5.16.1, 5.16.3 Mar 8, 2018
@DennisOSRM DennisOSRM deleted the fewer_json_copies branch November 6, 2022 14:11
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.

None yet

3 participants