-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add critical path scheduler to improve build times #2019
Conversation
The existing algorithm doesn't work because it strictly requires that all outputs are visited before updating an edge. So any task downstream from a task with multiple out-edges may get ignored. The fix is to always propagate your critical time to the next input node, and only place it in the queue if you offer a higher critical time.
active_edges.erase(e); | ||
|
||
for (std::vector<Node*>::iterator it = e->inputs_.begin(), | ||
end = e->inputs_.end(); |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed C++11 isn't required yet from the code style used elsewhere. Is that correct?
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also means no std::chrono::duration<int64_t, std::milli>
:-(
src/build.cc
Outdated
@@ -75,6 +75,16 @@ bool DryRunCommandRunner::WaitForCommand(Result* result) { | |||
|
|||
} // namespace | |||
|
|||
|
|||
bool EdgeQueue::EdgePriorityCompare::operator()(const Edge* e1, const Edge* e2) const { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
graph.h
which defines Edge
isn't included in the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could template it. (And if you really mean for it to be just Edge*
, you can static_assert that the template is a forward-declared type. As in
class Edge; // Fwd declare
...
template <typename EdgeType>
[[nodiscard]] bool operator()(const EdgeType* e1, const EdgeType* e2) const {
static_assert(std::is_same_v<EdgeType, Edge>); // Avoiding including `graph.h`.
...
50b5f28
to
aff5ebc
Compare
AddTarget cannot add edges to the ready queue before the critical time has been computed.
In my case, the initial build took the same time as with ninja master (I've rebased this PR). What I very much like is that I can manually tweak the ninja cmdline and add known long-running targets there; adding such an object file reduced the initial build time from 33 seconds to 26 seconds. Subsequent builds with FWIW, some info about my very imbalanced build (few long-running processes) with
|
ninja-build/ninja#2019, rebased onto latest master.
I should add that for primed builds with a |
Just to say that the current patch doesn't change the build time for Fuchsia builds (it may even slow it down by a few percent but it' s hard to be certain), however we rely heavily on pools, which may be limiting the benefit of the new algorithm. |
ninja-build/ninja#2019, rebased onto latest master (including ninja-build/ninja#1866 for a deterministic and predictable build order, which hasn't landed in any official release yet).
@peterbell10 where did you get such a beautiful chart? I've been staring at tables, waterfall charts and flame graphs, but I'd much prefer to see build-times your way. |
That's ninjatracing: https://github.com/nico/ninjatracing |
Closing in favor of #2177. |
Including ninja-build/ninja#2019 for adaptive scheduling.
Including ninja-build/ninja#2019 for adaptive scheduling.
Including ninja-build/ninja#2019 for adaptive scheduling.
FWIW, I've published a drop-in ninja release including this PR as main change, making the historical-build-times usage opt-in via |
@kinke - awesome, I'm going to try this soon. Just to clarify (since you can't create issues on forks), without |
Nope; there's still the default prioritization by explicit cmdline order / node depth. |
This is based on #1333 and fixes #376.
I'm interesting in seeing #1333 merged, but unfortunately it seems to have languished. When I tried it, it was completely broken. This maintains the same principal, but reworks it to: address review comments, fix bugs, and improve time complexity of operations.
For each edge depended on by the targets being built, we calculate its priority as the longest path through the weighted directed graph of dependencies. Where weights come from the build log's last execution time. Ready build tasks are then always executed highest priority first.
#1333 maintained a
std::list
of all edges in priority order. This PR instead uses astd::priority_queue
of only the ready edges (wrapped in theEdgeQueue
class). This method has log timepush
, andpop
so avoids theO(n)
traversal we had with the list.