From 88379eedeae654391711d8cdda17dfc2be6367ef Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Wed, 12 May 2021 10:46:21 +0200 Subject: Keep phase locked while working own queue --- libbuild2/adhoc-rule-buildscript.cxx | 4 +--- libbuild2/algorithm.cxx | 16 +++++----------- libbuild2/context.cxx | 30 +++++++++++++++++++----------- libbuild2/context.hxx | 8 ++++++-- libbuild2/context.ixx | 8 ++------ libbuild2/scheduler.cxx | 15 +++++---------- libbuild2/scheduler.hxx | 14 ++++++++++++++ libbuild2/scheduler.ixx | 35 +++++++++++++++++++++++++++++++++++ 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 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 + 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 + 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 r = wait_impl (start_count, task_count, wq)) + return *r; + + return suspend (start_count, task_count); + } + + template + 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 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 ()) -- cgit v1.1