Keep phase locked while working own queue

8 files changed, 87 insertions, 43 deletions

diff --git a/libbuild2/adhoc-rule-buildscript.cxx b/libbuild2/adhoc-rule-buildscript.cxx
index 168856c..73219b6 100644
--- a/libbuild2/adhoc-rule-buildscript.cxx
+++ b/libbuild2/adhoc-rule-buildscript.cxx
@@ -265,9 +265,7 @@ namespace build2
 
         const target& pt (*p.target);
 
-        const auto& tc (pt[a].task_count);
-        if (tc.load (memory_order_acquire) >= busy)
-          ctx.sched.wait (exec, tc, scheduler::work_none);
+        ctx.sched.wait (exec, pt[a].task_count, scheduler::work_none);
 
         target_state s (pt.executed_state (a));
         rs |= s;
diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index 5e93935..a19a6a0 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -236,8 +236,8 @@ namespace build2
         // to switch the phase to load. Which would result in a deadlock
         // unless we release the phase.
         //
-        phase_unlock ul (ct.ctx);
-        e = ctx.sched.wait (busy - 1, task_count, *wq);
+        phase_unlock u (ct.ctx, true /* unlock */, true /* delay */);
+        e = ctx.sched.wait (busy - 1, task_count, u, *wq);
       }
 
       // We don't lock already applied or executed targets.
@@ -2015,9 +2015,7 @@ namespace build2
 
       // If the target is still busy, wait for its completion.
       //
-      const auto& tc (mt[a].task_count);
-      if (tc.load (memory_order_acquire) >= busy)
-        ctx.sched.wait (exec, tc, scheduler::work_none);
+      ctx.sched.wait (exec, mt[a].task_count, scheduler::work_none);
 
       r |= mt.executed_state (a);
 
@@ -2067,9 +2065,7 @@ namespace build2
 
       const target& mt (*ts[i]);
 
-      const auto& tc (mt[a].task_count);
-      if (tc.load (memory_order_acquire) >= busy)
-        ctx.sched.wait (exec, tc, scheduler::work_none);
+      ctx.sched.wait (exec, mt[a].task_count, scheduler::work_none);
 
       r |= mt.executed_state (a);
 
@@ -2151,9 +2147,7 @@ namespace build2
 
       const target& pt (*p.target);
 
-      const auto& tc (pt[a].task_count);
-      if (tc.load (memory_order_acquire) >= busy)
-        ctx.sched.wait (exec, tc, scheduler::work_none);
+      ctx.sched.wait (exec, pt[a].task_count, scheduler::work_none);
 
       target_state s (pt.executed_state (a));
       rs |= s;
diff --git a/libbuild2/context.cxx b/libbuild2/context.cxx
index b4ba5df..3ae8a07 100644
--- a/libbuild2/context.cxx
+++ b/libbuild2/context.cxx
@@ -853,27 +853,35 @@ namespace build2
   // phase_unlock
   //
   phase_unlock::
-  phase_unlock (context& ctx, bool u)
-      : l (u ? phase_lock_instance : nullptr)
+  phase_unlock (context& c, bool u, bool d)
+      : ctx (u ? &c : nullptr), lock (nullptr)
   {
-    if (u)
+    if (u && !d)
+      unlock ();
+  }
+
+  void phase_unlock::
+  unlock ()
+  {
+    if (ctx != nullptr && lock == nullptr)
     {
-      assert (&l->ctx == &ctx);
+      lock = phase_lock_instance;
+      assert (&lock->ctx == ctx);
 
-      phase_lock_instance = nullptr; // Note: not l->prev.
-      ctx.phase_mutex.unlock (l->phase);
+      phase_lock_instance = nullptr; // Note: not lock->prev.
+      ctx->phase_mutex.unlock (lock->phase);
 
-      //text << this_thread::get_id () << " phase unlock  " << l->p;
+      //text << this_thread::get_id () << " phase unlock  " << lock->phase;
     }
   }
 
   phase_unlock::
   ~phase_unlock () noexcept (false)
   {
-    if (l != nullptr)
+    if (lock != nullptr)
     {
-      bool r (l->ctx.phase_mutex.lock (l->phase));
-      phase_lock_instance = l;
+      bool r (ctx->phase_mutex.lock (lock->phase));
+      phase_lock_instance = lock;
 
       // Fail unless we are already failing. Note that we keep the phase
       // locked since there will be phase_lock down the stack to unlock it.
@@ -881,7 +889,7 @@ namespace build2
       if (!r && !uncaught_exception ())
         throw failed ();
 
-      //text << this_thread::get_id () << " phase lock    " << l->p;
+      //text << this_thread::get_id () << " phase lock    " << lock->phase;
     }
   }
 
diff --git a/libbuild2/context.hxx b/libbuild2/context.hxx
index 0d401de..ad89f58 100644
--- a/libbuild2/context.hxx
+++ b/libbuild2/context.hxx
@@ -594,10 +594,14 @@ namespace build2
   //
   struct LIBBUILD2_SYMEXPORT phase_unlock
   {
-    phase_unlock (context&, bool unlock = true);
+    phase_unlock (context&, bool unlock = true, bool delay = false);
     ~phase_unlock () noexcept (false);
 
-    phase_lock* l;
+    void
+    unlock ();
+
+    context* ctx;
+    phase_lock* lock;
   };
 
   // Assuming we have a lock on the current phase, temporarily switch to a
diff --git a/libbuild2/context.ixx b/libbuild2/context.ixx
index 73601d4..4f86c28 100644
--- a/libbuild2/context.ixx
+++ b/libbuild2/context.ixx
@@ -56,12 +56,8 @@ namespace build2
   inline void wait_guard::
   wait ()
   {
-    if (task_count->load (memory_order_acquire) > start_count)
-    {
-      phase_unlock u (*ctx, phase);
-      ctx->sched.wait (start_count, *task_count);
-    }
-
+    phase_unlock u (*ctx, phase, true /* delay */);
+    ctx->sched.wait (start_count, *task_count, u);
     task_count = nullptr;
   }
 }
diff --git a/libbuild2/scheduler.cxx b/libbuild2/scheduler.cxx
index deb5399..43da681 100644
--- a/libbuild2/scheduler.cxx
+++ b/libbuild2/scheduler.cxx
@@ -50,16 +50,9 @@ namespace build2
     scheduler_queue = q;
   }
 
-  size_t scheduler::
-  wait (size_t start_count, const atomic_count& task_count, work_queue wq)
+  optional<size_t> scheduler::
+  wait_impl (size_t start_count, const atomic_count& task_count, work_queue wq)
   {
-    // Note that task_count is a synchronization point.
-    //
-    size_t tc;
-
-    if ((tc = task_count.load (memory_order_acquire)) <= start_count)
-      return tc;
-
     assert (max_active_ != 1); // Serial execution, nobody to wait for.
 
     // See if we can run some of our own tasks.
@@ -71,6 +64,8 @@ namespace build2
       //
       if (task_queue* tq = queue ())
       {
+        size_t tc;
+
         for (lock ql (tq->mutex); !tq->shutdown && !empty_back (*tq); )
         {
           pop_back (*tq, ql);
@@ -91,7 +86,7 @@ namespace build2
       }
     }
 
-    return suspend (start_count, task_count);
+    return nullopt;
   }
 
   void scheduler::
diff --git a/libbuild2/scheduler.hxx b/libbuild2/scheduler.hxx
index e1bb715..9556caa 100644
--- a/libbuild2/scheduler.hxx
+++ b/libbuild2/scheduler.hxx
@@ -125,6 +125,16 @@ namespace build2
       return wait (0, task_count, wq);
     }
 
+    // As above but call lock.unlock() before suspending (can be used to
+    // unlock the phase).
+    //
+    template <typename L>
+    size_t
+    wait (size_t start_count,
+          const atomic_count& task_count,
+          L& lock,
+          work_queue = work_all);
+
     // Mark the queue so that we don't work any tasks that may already be
     // there. In the normal "bunch of acync() calls followed by wait()"
     // cases this happens automatically but in special cases where async()
@@ -846,6 +856,10 @@ namespace build2
 
     static void
     queue (task_queue*) noexcept;
+
+  private:
+    optional<size_t>
+    wait_impl (size_t, const atomic_count&, work_queue);
   };
 }
 
diff --git a/libbuild2/scheduler.ixx b/libbuild2/scheduler.ixx
index f9f0f2e..4cf347c 100644
--- a/libbuild2/scheduler.ixx
+++ b/libbuild2/scheduler.ixx
@@ -3,6 +3,41 @@
 
 namespace build2
 {
+  inline size_t scheduler::
+  wait (size_t start_count, const atomic_count& task_count, work_queue wq)
+  {
+    // Note that task_count is a synchronization point.
+    //
+    size_t tc;
+    if ((tc = task_count.load (memory_order_acquire)) <= start_count)
+      return tc;
+
+    if (optional<size_t> r = wait_impl (start_count, task_count, wq))
+      return *r;
+
+    return suspend (start_count, task_count);
+  }
+
+  template <typename L>
+  inline size_t scheduler::
+  wait (size_t start_count,
+        const atomic_count& task_count,
+        L& lock,
+        work_queue wq)
+  {
+    // Note that task_count is a synchronization point.
+    //
+    size_t tc;
+    if ((tc = task_count.load (memory_order_acquire)) <= start_count)
+      return tc;
+
+    if (optional<size_t> r = wait_impl (start_count, task_count, wq))
+      return *r;
+
+    lock.unlock ();
+    return suspend (start_count, task_count);
+  }
+
   inline scheduler::queue_mark::
   queue_mark (scheduler& s)
       : tq_ (s.queue ())