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 dummy edges before inplace permutation #5023

Merged
merged 1 commit into from
Apr 19, 2018

Conversation

oxidase
Copy link
Contributor

@oxidase oxidase commented Apr 12, 2018

Issue

#5021 (comment) points that the check edge_list.size() < std::numeric_limits<EdgeID>::max() can be placed before the loop, but this check limits number of valid edges by 2^32 - #dummy_edges.

PR implements a removal of dummy edges before a util::inplacePermutation call:
✔️ number of edges is limited by 2^32

❌ an additional iteration over the list of edges, but potentially linearized removal can be faster due to caching

/cc @danpat

Tasklist

Requirements / Relations

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

@oxidase oxidase added the Review label Apr 12, 2018
@TheMarex
Copy link
Member

The reason why we don't remove the dummy edges is that it impacts performance significantly during contraction, since we need the over-allocation for the DynamicGraph to be effective. For example on Bayern the contraction on master needs 504 sec but with this branch 549 sec. If we need to limit the amount of over-allocation we should add a fixed ratio of 30% over-allocation.

@oxidase oxidase force-pushed the renumber-remove-dummy-edges branch from 812fe20 to 4c65210 Compare April 17, 2018 20:15
@oxidase
Copy link
Contributor Author

oxidase commented Apr 17, 2018

@TheMarex thank you for running performance benchmarks! I think it is enough for now to keep a sanity check to avoid overflows of permutation indexes.

I don't think we need to limit over-allocation but a check before

graph.InsertEdge(edge.source, edge.target, edge.data);

that will stop contraction like

                if (edge_list.size() >= std::numeric_limits<EdgeID>::max())
                {
                    util::Log(logWARNING) << "Too many sortcuts, exiting graph contraction";
                    node_data.Renumber(new_to_old_node_id);
                    RenumberGraph(graph, new_to_old_node_id);
                    return std::move(node_data.is_core);
                }

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.

Sanity check looks good. 👍

@TheMarex TheMarex merged commit b2aeb47 into master Apr 19, 2018
@DennisOSRM DennisOSRM deleted the renumber-remove-dummy-edges branch November 6, 2022 14:13
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