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

Avoid copying ManyToMany table results #5923

Merged
merged 1 commit into from
Jan 4, 2021

Conversation

mjjbell
Copy link
Member

@mjjbell mjjbell commented Jan 1, 2021

Issue

Regardless of any copy elision on the returned pair value, the duration and distance results are always copied.

Fix this by passing rvalue references to std::make_pair.

This could save copying a few megabytes on Table and Trip requests with hundreds of sources/destinations and waypoints respectively. I haven't profiled this to see if this affects performance in any other way.

Tasklist

Regardless of any copy elision on the returned pair value, the
duration and distance results are always copied.

Fix this by passing rvalue references to std::make_pair.
Copy link

@jcoupey jcoupey left a comment

Choose a reason for hiding this comment

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

Looks like a nice low-hanging fruit optimization. @mjjbell have you been able to notice a significant computing time reduction?

One of the CI checks is failing but I don't have any clue on that...

@danpat
Copy link
Member

danpat commented Jan 4, 2021

The big benefit here is probably going to be reduce memory footprint under heavy load when making lots of concurrent matrix requests - I suspect the overall performance improvement will be really small. The copy only happens once per request, not within the compute intensive routing loop.

Definitely a good change though, 👍

I think the CI failure is good to ignore - it's a coverage check, which seem to be a bit noisy when only a couple of lines change.

@danpat danpat merged commit 58ba3fc into Project-OSRM:master Jan 4, 2021
@mjjbell
Copy link
Member Author

mjjbell commented Jan 4, 2021

@jcoupey sorry I don't have any numbers. I just noticed it and thought it would be a quick win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants