Merge pull request #962 from sgraham/pool-use-fix

Fix pool use count going unbalanced

4 files changed, 43 insertions, 21 deletions

diff --git a/src/build.cc b/src/build.cc
index ccdb37f..cdb8e0a 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -374,21 +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) {
   map<Edge*, bool>::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
-  ResumeDelayedJobs(edge);
+  // See if this job frees up any delayed jobs.
+  if (directly_wanted)
+    edge->pool()->EdgeFinished(*edge);
+  edge->pool()->RetrieveReadyEdges(&ready_);
 
   // Check off any nodes we were waiting for with this edge.
   for (vector<Node*>::iterator o = edge->outputs_.begin();
diff --git a/src/build.h b/src/build.h
index 4b48c5f..8106faa 100644
--- a/src/build.h
+++ b/src/build.h
@@ -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
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; }