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

ComputeCriticalPath: Use topological sort to speed up function. #2443

Merged

Conversation

digit-google
Copy link
Contributor

Use a topological sort to get a sorted list of edges to perform the critical path weigth propagation as fast as possible.

The previous version used a data flow algorithm that forced the values to be recomputed several times for far too many edges on complex build plans.

For example, for a Fuchsia build plan with 93339 edges, this reduces the ComputeCriticalPath metric from 1.9s to 80ms!

The unit-tests still pass, and manual testing shows that the order of commands does not change before and after this change for the example build plan above.

@digit-google
Copy link
Contributor Author

invoking @bradking and @jhasse here.

I must say I am very surprised by the performance difference, but I can't see anything wrong in the new code :-/
Let me know if you spot a mistake in the algorithm!

@jhasse
Copy link
Collaborator

jhasse commented May 7, 2024

Awesome!

@peterbell10 can you have a look?

Copy link
Contributor

@peterbell10 peterbell10 left a comment

Choose a reason for hiding this comment

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

Makes sense to me, just a couple small comments.

src/build.cc Outdated
Comment on lines 533 to 535
for (auto reverse_it = sorted_edges.end();
reverse_it != sorted_edges.begin();) {
Edge* edge = *(--reverse_it);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT:

Suggested change
for (auto reverse_it = sorted_edges.end();
reverse_it != sorted_edges.begin();) {
Edge* edge = *(--reverse_it);
for (auto reverse_it = sorted_edges.rbegin();
reverse_it != sorted_edges.rend(); ++reverse_it) {
Edge* edge = *reverse_it;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I always forget about these. Thanks.

src/build.cc Outdated

// First, reset all weights to 1.
for (Edge* edge : sorted_edges)
edge->set_critical_path_weight(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
edge->set_critical_path_weight(1);
edge->set_critical_path_weight(EdgeWeightHeuristic(edge));

Copy link
Contributor

Choose a reason for hiding this comment

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

Edge weight for phony edges is 0, so this is a meaningful change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point! Done.

Use a topological sort to get a sorted list of edges to perform
the critical path weigth propagation as fast as possible.

The previous version used a data flow algorithm that forced
the values to be recomputed several times for far too many
edges on complex build plans.

For example, for a Fuchsia build plan with 93339 edges,
this reduces the ComputeCriticalPath metric from 1.9s to 80ms!

The unit-tests still pass, and manual testing shows that the
order of commands does not change before and after this change
for the example build plan above.
@digit-google digit-google force-pushed the critical-path-with-topo-sort branch from 982848d to ef89584 Compare May 9, 2024 08:26
@jhasse jhasse merged commit c470bf7 into ninja-build:master May 11, 2024
11 checks passed
@jhasse jhasse modified the milestones: 1.13.0, 1.12.1 May 11, 2024
@digit-google digit-google deleted the critical-path-with-topo-sort branch May 13, 2024 09:04
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