From 6652258c2fa510f043d32be95ef4c44eaa2800dc Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 23 Apr 2015 15:54:28 -0700 Subject: Fix pool use count going unbalanced --- src/build.cc | 4 +++- src/build_test.cc | 44 ++++++++++++++++++++++++++++++++++++-------- src/state.h | 1 + 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/build.cc b/src/build.cc index ccdb37f..f14831e 100644 --- a/src/build.cc +++ b/src/build.cc @@ -411,7 +411,9 @@ void Plan::NodeFinished(Node* node) { ScheduleWork(*oe); } else { // We do not need to build this edge, but we might need to build one of - // its dependents. + // its dependents. Make sure the pool schedules it before it's finished + // otherwise the pool use count may be invalid. + (*oe)->pool()->EdgeScheduled(**oe); EdgeFinished(*oe); } } diff --git a/src/build_test.cc b/src/build_test.cc index a313693..52a17c9 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -495,8 +495,8 @@ struct BuildTest : public StateTestWithBuiltinRules, public BuildLogUser { /// State of command_runner_ and logs contents (if specified) ARE MODIFIED. /// Handy to check for NOOP builds, and higher-level rebuild tests. void RebuildTarget(const string& target, const char* manifest, - const char* log_path = NULL, - const char* deps_path = NULL); + const char* log_path = NULL, const char* deps_path = NULL, + State* state = NULL); // Mark a path dirty. void Dirty(const string& path); @@ -516,10 +516,13 @@ struct BuildTest : public StateTestWithBuiltinRules, public BuildLogUser { }; void BuildTest::RebuildTarget(const string& target, const char* manifest, - const char* log_path, const char* deps_path) { - State state; - ASSERT_NO_FATAL_FAILURE(AddCatRule(&state)); - AssertParse(&state, manifest); + const char* log_path, const char* deps_path, + State* state) { + State local_state, *pstate = &local_state; + if (state) + pstate = state; + ASSERT_NO_FATAL_FAILURE(AddCatRule(pstate)); + AssertParse(pstate, manifest); string err; BuildLog build_log, *pbuild_log = NULL; @@ -532,13 +535,13 @@ void BuildTest::RebuildTarget(const string& target, const char* manifest, DepsLog deps_log, *pdeps_log = NULL; if (deps_path) { - ASSERT_TRUE(deps_log.Load(deps_path, &state, &err)); + ASSERT_TRUE(deps_log.Load(deps_path, pstate, &err)); ASSERT_TRUE(deps_log.OpenForWrite(deps_path, &err)); ASSERT_EQ("", err); pdeps_log = &deps_log; } - Builder builder(&state, config_, pbuild_log, pdeps_log, &fs_); + Builder builder(pstate, config_, pbuild_log, pdeps_log, &fs_); EXPECT_TRUE(builder.AddTarget(target, &err)); command_runner_.commands_ran_.clear(); @@ -1092,6 +1095,31 @@ TEST_F(BuildTest, SwallowFailuresLimit) { ASSERT_EQ("cannot make progress due to previous errors", err); } +TEST_F(BuildTest, PoolEdgesReadyButNotWanted) { + fs_.Create("x", ""); + + const char* manifest = + "pool some_pool\n" + " depth = 4\n" + "rule touch\n" + " command = touch $out\n" + " pool = some_pool\n" + "rule cc\n" + " command = touch grit\n" + "\n" + "build B.d.stamp: cc | x\n" + "build C.stamp: touch B.d.stamp\n" + "build final.stamp: touch || C.stamp\n"; + + RebuildTarget("final.stamp", manifest); + + fs_.RemoveFile("B.d.stamp"); + + State save_state; + RebuildTarget("final.stamp", manifest, NULL, NULL, &save_state); + EXPECT_GE(save_state.LookupPool("some_pool")->current_use(), 0); +} + struct BuildWithLogTest : public BuildTest { BuildWithLogTest() { builder_.SetBuildLog(&build_log_); diff --git a/src/state.h b/src/state.h index 5000138..d7987ba 100644 --- a/src/state.h +++ b/src/state.h @@ -44,6 +44,7 @@ struct Pool { bool is_valid() const { return depth_ >= 0; } int depth() const { return depth_; } const string& name() const { return name_; } + int current_use() const { return current_use_; } /// true if the Pool might delay this edge bool ShouldDelayEdge() const { return depth_ != 0; } -- cgit v0.12 From dcdd042ca93fd0057d7e1b0eedfbab49790b2e00 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 24 Apr 2015 14:25:17 -0700 Subject: add comment --- src/state.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/state.h b/src/state.h index d7987ba..58fac13 100644 --- a/src/state.h +++ b/src/state.h @@ -71,6 +71,9 @@ struct Pool { /// |current_use_| is the total of the weights of the edges which are /// currently scheduled in the Plan (i.e. the edges in Plan::ready_). + /// This is generally <= to depth_. It can exceed it very briefly when the + /// pool is notified about an edge that's about to be finished that will + /// not actually be started. See Plan::NodeFinished(). int current_use_; int depth_; -- cgit v0.12 From 4c4887d26011225deaa20b6752193be0293624ab Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 24 Apr 2015 16:06:55 -0700 Subject: avoid calling ResumeDelayedJobs instead --- src/build.cc | 13 +++++++------ src/build.h | 2 +- src/state.h | 3 --- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/build.cc b/src/build.cc index f14831e..e266b5c 100644 --- a/src/build.cc +++ b/src/build.cc @@ -379,7 +379,7 @@ void Plan::ResumeDelayedJobs(Edge* edge) { edge->pool()->RetrieveReadyEdges(&ready_); } -void Plan::EdgeFinished(Edge* edge) { +void Plan::EdgeFinished(Edge* edge, bool directly_wanted) { map::iterator e = want_.find(edge); assert(e != want_.end()); if (e->second) @@ -388,7 +388,10 @@ void Plan::EdgeFinished(Edge* edge) { edge->outputs_ready_ = true; // See if this job frees up any delayed jobs - ResumeDelayedJobs(edge); + if (directly_wanted) + ResumeDelayedJobs(edge); + else + edge->pool()->RetrieveReadyEdges(&ready_); // Check off any nodes we were waiting for with this edge. for (vector::iterator o = edge->outputs_.begin(); @@ -411,10 +414,8 @@ void Plan::NodeFinished(Node* node) { ScheduleWork(*oe); } else { // We do not need to build this edge, but we might need to build one of - // its dependents. Make sure the pool schedules it before it's finished - // otherwise the pool use count may be invalid. - (*oe)->pool()->EdgeScheduled(**oe); - EdgeFinished(*oe); + // its dependents. + EdgeFinished(*oe, want_e->second); } } } diff --git a/src/build.h b/src/build.h index 4b48c5f..6eabae5 100644 --- a/src/build.h +++ b/src/build.h @@ -58,7 +58,7 @@ struct Plan { /// Mark an edge as done building. Used internally and by /// tests. - void EdgeFinished(Edge* edge); + void EdgeFinished(Edge* edge, bool directly_wanted = true); /// Clean the given node during the build. /// Return false on error. diff --git a/src/state.h b/src/state.h index 58fac13..d7987ba 100644 --- a/src/state.h +++ b/src/state.h @@ -71,9 +71,6 @@ struct Pool { /// |current_use_| is the total of the weights of the edges which are /// currently scheduled in the Plan (i.e. the edges in Plan::ready_). - /// This is generally <= to depth_. It can exceed it very briefly when the - /// pool is notified about an edge that's about to be finished that will - /// not actually be started. See Plan::NodeFinished(). int current_use_; int depth_; -- cgit v0.12 From bf2f4b7356c8ea918718651bae586149ab97ebee Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Mon, 27 Apr 2015 11:12:43 -0700 Subject: simplify & inline --- src/build.cc | 19 +++++++------------ src/build.h | 7 +------ 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/build.cc b/src/build.cc index e266b5c..cdb8e0a 100644 --- a/src/build.cc +++ b/src/build.cc @@ -374,24 +374,19 @@ void Plan::ScheduleWork(Edge* edge) { } } -void Plan::ResumeDelayedJobs(Edge* edge) { - edge->pool()->EdgeFinished(*edge); - edge->pool()->RetrieveReadyEdges(&ready_); -} - -void Plan::EdgeFinished(Edge* edge, bool directly_wanted) { +void Plan::EdgeFinished(Edge* edge) { map::iterator e = want_.find(edge); assert(e != want_.end()); - if (e->second) + bool directly_wanted = e->second; + if (directly_wanted) --wanted_edges_; want_.erase(e); edge->outputs_ready_ = true; - // See if this job frees up any delayed jobs + // See if this job frees up any delayed jobs. if (directly_wanted) - ResumeDelayedJobs(edge); - else - edge->pool()->RetrieveReadyEdges(&ready_); + edge->pool()->EdgeFinished(*edge); + edge->pool()->RetrieveReadyEdges(&ready_); // Check off any nodes we were waiting for with this edge. for (vector::iterator o = edge->outputs_.begin(); @@ -415,7 +410,7 @@ void Plan::NodeFinished(Node* node) { } else { // We do not need to build this edge, but we might need to build one of // its dependents. - EdgeFinished(*oe, want_e->second); + EdgeFinished(*oe); } } } diff --git a/src/build.h b/src/build.h index 6eabae5..8106faa 100644 --- a/src/build.h +++ b/src/build.h @@ -58,7 +58,7 @@ struct Plan { /// Mark an edge as done building. Used internally and by /// tests. - void EdgeFinished(Edge* edge, bool directly_wanted = true); + void EdgeFinished(Edge* edge); /// Clean the given node during the build. /// Return false on error. @@ -78,11 +78,6 @@ private: /// currently-full pool. void ScheduleWork(Edge* edge); - /// Allows jobs blocking on |edge| to potentially resume. - /// For example, if |edge| is a member of a pool, calling this may schedule - /// previously pending jobs in that pool. - void ResumeDelayedJobs(Edge* edge); - /// Keep track of which edges we want to build in this plan. If this map does /// not contain an entry for an edge, we do not want to build the entry or its /// dependents. If an entry maps to false, we do not want to build it, but we -- cgit v0.12