Un-tune scheduler when building build system modules

8 files changed, 167 insertions, 100 deletions

diff --git a/libbuild2/context.hxx b/libbuild2/context.hxx
index 0a2deea..e720321 100644
--- a/libbuild2/context.hxx
+++ b/libbuild2/context.hxx
@@ -131,6 +131,9 @@ namespace build2
   // future we can do save/restore but then we would need some indication that
   // we have switched to another task).
   //
+  // Note that sharing the same scheduler between multiple top-level contexts
+  // can currently be problematic due to operation-specific scheduler tuning.
+  //
   // The loaded_modules state (module.hxx) is shared among all the contexts
   // (there is no way to have multiple shared library loading "contexts") and
   // is protected by loaded_modules_lock. A nested context should normally
diff --git a/libbuild2/install/operation.cxx b/libbuild2/install/operation.cxx
index a2ad7d0..4515f14 100644
--- a/libbuild2/install/operation.cxx
+++ b/libbuild2/install/operation.cxx
@@ -39,7 +39,7 @@ namespace build2
       "installed",
       "has nothing to install", // We cannot "be installed".
       execution_mode::first,
-      0,
+      0 /* concurrency */,      // Run serially.
       &install_pre,
       nullptr
     };
@@ -62,7 +62,7 @@ namespace build2
       "uninstalled",
       "is not installed",
       execution_mode::last,
-      0,
+      0 /* concurrency */,      // Run serially
       &install_pre,
       nullptr
     };
diff --git a/libbuild2/module.cxx b/libbuild2/module.cxx
index 45cbd30..b73ddb3 100644
--- a/libbuild2/module.cxx
+++ b/libbuild2/module.cxx
@@ -243,11 +243,25 @@ namespace build2
           diag_frame::stack_guard diag_cutoff (nullptr);
 
           auto df = make_diag_frame (
-            [&loc, &mod](const diag_record& dr)
+            [&loc, &mod] (const diag_record& dr)
             {
               dr << info (loc) << "while loading build system module " << mod;
             });
 
+          // Un-tune the scheduler.
+          //
+          // Note that we can only do this if we are running serially because
+          // otherwise we cannot guarantee the scheduler is idle (we could
+          // have waiting threads from the outer context). This is fine for
+          // now since the only two tuning level we use are serial and full
+          // concurrency (turns out currently we don't really need this: we
+          // will always be called during load or match phases and we always
+          // do parallel match; but let's keep it in case things change).
+          //
+          auto sched_tune (ctx.sched.serial ()
+                           ? scheduler::tune_guard (ctx.sched, 0)
+                           : scheduler::tune_guard ());
+
           // Note that for now we suppress progress since it would clash with
           // the progress of what we are already doing (maybe in the future we
           // can do save/restore but then we would need some sort of
diff --git a/libbuild2/operation.cxx b/libbuild2/operation.cxx
index 0bf87d5..a9b6107 100644
--- a/libbuild2/operation.cxx
+++ b/libbuild2/operation.cxx
@@ -271,100 +271,105 @@ namespace build2
     if (ctx.current_mode == execution_mode::last)
       reverse (ts.begin (), ts.end ());
 
-    // Tune the scheduler.
-    //
-    switch (ctx.current_inner_oif->concurrency)
-    {
-    case 0: ctx.sched.tune (1); break;          // Run serially.
-    case 1:                     break;          // Run as is.
-    default:                    assert (false); // Not yet supported.
-    }
-
     phase_lock pl (ctx, run_phase::execute); // Never switched.
 
-    // Set the dry-run flag.
-    //
-    ctx.dry_run = ctx.dry_run_option;
-
-    // Setup progress reporting if requested.
-    //
-    string what; // Note: must outlive monitor_guard.
-    scheduler::monitor_guard mg;
-
-    if (prog && show_progress (1 /* max_verb */))
     {
-      size_t init (ctx.target_count.load (memory_order_relaxed));
-      size_t incr (init > 100 ? init / 100 : 1); // 1%.
+      // Tune the scheduler.
+      //
+      using tune_guard = scheduler::tune_guard;
+      tune_guard sched_tune;
 
-      if (init != incr)
+      switch (ctx.current_inner_oif->concurrency)
       {
-        what = "% of targets " + diag_did (ctx, a);
+      case 0: sched_tune = tune_guard (ctx.sched, 1); break; // Run serially.
+      case 1:                                         break; // Run as is.
+      default:                               assert (false); // Not supported.
+      }
 
-        mg = ctx.sched.monitor (
-          ctx.target_count,
-          init - incr,
-          [init, incr, &what, &ctx] (size_t c) -> size_t
-          {
-            size_t p ((init - c) * 100 / init);
-            size_t s (ctx.skip_count.load (memory_order_relaxed));
+      // Set the dry-run flag.
+      //
+      ctx.dry_run = ctx.dry_run_option;
 
-            diag_progress_lock pl;
-            diag_progress  = ' ';
-            diag_progress += to_string (p);
-            diag_progress += what;
+      // Setup progress reporting if requested.
+      //
+      string what; // Note: must outlive monitor_guard.
+      scheduler::monitor_guard mg;
 
-            if (s != 0)
-            {
-              diag_progress += " (";
-              diag_progress += to_string (s);
-              diag_progress += " skipped)";
-            }
+      if (prog && show_progress (1 /* max_verb */))
+      {
+        size_t init (ctx.target_count.load (memory_order_relaxed));
+        size_t incr (init > 100 ? init / 100 : 1); // 1%.
 
-            return c - incr;
-          });
-      }
-    }
+        if (init != incr)
+        {
+          what = "% of targets " + diag_did (ctx, a);
 
-    // Similar logic to execute_members(): first start asynchronous execution
-    // of all the top-level targets.
-    //
-    {
-      atomic_count task_count (0);
-      wait_guard wg (ctx, task_count);
+          mg = ctx.sched.monitor (
+            ctx.target_count,
+            init - incr,
+            [init, incr, &what, &ctx] (size_t c) -> size_t
+            {
+              size_t p ((init - c) * 100 / init);
+              size_t s (ctx.skip_count.load (memory_order_relaxed));
+
+              diag_progress_lock pl;
+              diag_progress  = ' ';
+              diag_progress += to_string (p);
+              diag_progress += what;
+
+              if (s != 0)
+              {
+                diag_progress += " (";
+                diag_progress += to_string (s);
+                diag_progress += " skipped)";
+              }
+
+              return c - incr;
+            });
+        }
+      }
 
-      for (const action_target& at: ts)
+      // Similar logic to execute_members(): first start asynchronous
+      // execution of all the top-level targets.
+      //
       {
-        const target& t (at.as_target ());
+        atomic_count task_count (0);
+        wait_guard wg (ctx, task_count);
 
-        l5 ([&]{trace << diag_doing (a, t);});
+        for (const action_target& at: ts)
+        {
+          const target& t (at.as_target ());
 
-        target_state s (execute_async (a, t, 0, task_count, false));
+          l5 ([&]{trace << diag_doing (a, t);});
 
-        // Bail out if the target has failed and we weren't instructed to keep
-        // going.
-        //
-        if (s == target_state::failed && !ctx.keep_going)
-          break;
-      }
+          target_state s (execute_async (a, t, 0, task_count, false));
 
-      wg.wait ();
-    }
+          // Bail out if the target has failed and we weren't instructed to
+          // keep going.
+          //
+          if (s == target_state::failed && !ctx.keep_going)
+            break;
+        }
 
-    // We are now running serially.
-    //
+        wg.wait ();
+      }
 
-    ctx.sched.tune (0); // Restore original scheduler settings.
+      // We are now running serially.
+      //
 
-    // Clear the dry-run flag.
-    //
-    ctx.dry_run = false;
+      // Clear the dry-run flag.
+      //
+      ctx.dry_run = false;
 
-    // Clear the progress if present.
-    //
-    if (mg)
-    {
-      diag_progress_lock pl;
-      diag_progress.clear ();
+      // Clear the progress if present.
+      //
+      if (mg)
+      {
+        diag_progress_lock pl;
+        diag_progress.clear ();
+      }
+
+      // Restore original scheduler settings.
     }
 
     // Print skip count if not zero. Note that we print it regardless of the
@@ -582,7 +587,7 @@ namespace build2
     "",
     "",
     execution_mode::first,
-    1,
+    1 /* concurrency */,
     nullptr,
     nullptr
   };
@@ -606,7 +611,7 @@ namespace build2
     "updated",
     "is up to date",
     execution_mode::first,
-    1,
+    1 /* concurrency */,
     nullptr,
     nullptr
   };
@@ -620,7 +625,7 @@ namespace build2
     "cleaned",
     "is clean",
     execution_mode::last,
-    1,
+    1 /* concurrency */,
     nullptr,
     nullptr
   };
diff --git a/libbuild2/operation.hxx b/libbuild2/operation.hxx
index 520b37b..125c692 100644
--- a/libbuild2/operation.hxx
+++ b/libbuild2/operation.hxx
@@ -221,8 +221,11 @@ namespace build2
 
     const execution_mode mode;
 
-    // This is the operation's concurrency multiplier. 0 means run serially,
-    // 1 means run at hardware concurrency (unless overridden by the user).
+    // This is the operation's concurrency multiplier. 0 means run serially, 1
+    // means run at hardware concurrency (or the concurrency specified by the
+    // user).
+    //
+    // Note: 0 and 1 are currently the only valid values.
     //
     const size_t concurrency;
 
diff --git a/libbuild2/scheduler.cxx b/libbuild2/scheduler.cxx
index 3c06176..bd2694c 100644
--- a/libbuild2/scheduler.cxx
+++ b/libbuild2/scheduler.cxx
@@ -125,7 +125,7 @@ namespace build2
     else if (active_ == 0 && external_ == 0)
     {
       // Note that we tried to handle this directly in this thread but that
-      // wouldn't work for the phase lock case where we cal; deactivate and
+      // wouldn't work for the phase lock case where we call deactivate and
       // then go wait on a condition variable: we would be doing deadlock
       // detection while holding the lock that prevents other threads from
       // making progress! So it has to be a separate monitoring thread.
@@ -391,24 +391,35 @@ namespace build2
       dead_thread_ = thread (deadlock_monitor, this);
   }
 
-  void scheduler::
+  size_t scheduler::
   tune (size_t max_active)
   {
     // Note that if we tune a parallel scheduler to run serially, we will
     // still have the deadlock monitoring thread running.
 
+    // With multiple initial active threads we will need to make changes to
+    // max_active_ visible to other threads and which we currently say can be
+    // accessed between startup and shutdown without a lock.
+    //
+    assert (init_active_ == 1);
+
     if (max_active == 0)
       max_active = orig_max_active_;
 
-    assert (max_active >= init_active_ &&
-            max_active <= orig_max_active_);
+    if (max_active != max_active_)
+    {
+      assert (max_active >= init_active_ &&
+              max_active <= orig_max_active_);
+
+      // The scheduler must not be active though some threads might still be
+      // comming off from finishing a task. So we busy-wait for them.
+      //
+      lock l (wait_idle ());
 
-    // The scheduler must not be active though some threads might still be
-    // comming off from finishing a task. So we busy-wait for them.
-    //
-    lock l (wait_idle ());
+      swap (max_active_, max_active);
+    }
 
-    max_active_ = max_active;
+    return max_active == orig_max_active_ ? 0 : max_active;
   }
 
   auto scheduler::
diff --git a/libbuild2/scheduler.hxx b/libbuild2/scheduler.hxx
index 0107753..4495cb9 100644
--- a/libbuild2/scheduler.hxx
+++ b/libbuild2/scheduler.hxx
@@ -209,17 +209,46 @@ namespace build2
 
     // Tune a started up scheduler.
     //
-    // Currently one cannot increase the number of max_active. Pass 0 to
-    // restore the initial value.
+    // Currently one cannot increase the number of (initial) max_active, only
+    // decrease it. Pass 0 to restore the initial value. Returns the old
+    // value (0 if it is initial).
     //
     // Note that tuning can only be done while the scheduler is inactive, that
-    // is, no threads are executing a task or are suspended. For example, in a
+    // is, no threads are executing or waiting on a task. For example, in a
     // setup with a single initial active thread that would be after a return
-    // from the top-level wait() call.
+    // from the top-level wait() call. Tuning the scheduler with more than one
+    // initial active threads is currently not supported.
     //
-    void
+    size_t
     tune (size_t max_active);
 
+    struct tune_guard
+    {
+      tune_guard (): s_ (nullptr) {}
+      tune_guard (scheduler& s, size_t ma): s_ (&s), o_ (s_->tune (ma)) {}
+      tune_guard (tune_guard&& x): s_ (x.s_), o_ (x.o_) {x.s_ = nullptr;}
+      tune_guard& operator= (tune_guard&& x)
+      {
+        if (&x != this)
+        {
+          s_ = x.s_;
+          o_ = x.o_;
+          x.s_ = nullptr;
+        }
+        return *this;
+      }
+
+      ~tune_guard ()
+      {
+        if (s_ != nullptr)
+          s_->tune (o_);
+      }
+
+    private:
+      scheduler* s_;
+      size_t o_;
+    };
+
     // Return true if the scheduler is configured to run tasks serially.
     //
     // Note: can only be called from threads that have observed startup.
@@ -415,8 +444,10 @@ namespace build2
     //                  active <= max_active
     // (init_active + helpers) <= max_threads (soft; see activate_helper())
     //
-    // Note that the first three are immutable between startup() and
-    // shutdown() so can be accessed without a lock (but see join()).
+    // Note that the first three are immutable between the startup() and
+    // shutdown() calls so can be accessed without a lock (but see join() and
+    // except for max_active_ which can be changed by tune() but only when the
+    // scheduler is idle).
     //
     size_t init_active_ = 0; // Initially active threads.
     size_t max_active_  = 0; // Maximum number of active threads.
@@ -430,7 +461,7 @@ namespace build2
     //
     size_t active_   = 0;  // Active master threads executing a task.
     size_t idle_     = 0;  // Idle helper threads waiting for a task.
-    size_t waiting_  = 0;  // Suspended master threads waiting for their tasks.
+    size_t waiting_  = 0;  // Suspended master threads waiting on their tasks.
     size_t ready_    = 0;  // Ready master thread waiting to become active.
     size_t starting_ = 0;  // Helper threads starting up.
 
diff --git a/libbuild2/test/operation.cxx b/libbuild2/test/operation.cxx
index 0ca8da0..061b2cc 100644
--- a/libbuild2/test/operation.cxx
+++ b/libbuild2/test/operation.cxx
@@ -33,7 +33,7 @@ namespace build2
       "tested",
       "has nothing to test", // We cannot "be tested".
       execution_mode::first,
-      1,
+      1 /* concurrency */,
       &test_pre,
       nullptr
     };