From 9a592abd7c7508386666bdbcd00d8bc030bc966e Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Fri, 15 Sep 2023 07:11:16 -0700 Subject: [PATCH] absl: optimize Condition checks in Mutex code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Remove special handling of Condition::kTrue. Condition::kTrue is used very rarely (frequently its uses even indicate confusion and bugs). But we pay few additional branches for kTrue on all Condition operations. Remove that special handling and simplify logic. 2. And remove known_false condition in Mutex code. Checking known_false condition only causes slow down because: 1. We already built skip list with equivalent conditions (and keep improving it on every Skip call). And when we built the skip list, we used more capable GuaranteedEqual function (it does not just check for equality of pointers, but for also for equality of function/arg). 2. Condition pointer are rarely equal even for equivalent conditions becuase temp Condition objects are usually created on the stack. We could call GuaranteedEqual(cond, known_false) instead of cond == known_false, but that slows down things even more (see point 1). So remove the known_false optimization. Benchmark results for this and the previous change: name old cpu/op new cpu/op delta BM_ConditionWaiters/0/1 36.0ns ± 0% 34.9ns ± 0% -3.02% (p=0.008 n=5+5) BM_ConditionWaiters/1/1 36.0ns ± 0% 34.9ns ± 0% -2.98% (p=0.008 n=5+5) BM_ConditionWaiters/2/1 35.9ns ± 0% 34.9ns ± 0% -3.03% (p=0.016 n=5+4) BM_ConditionWaiters/0/8 55.5ns ± 5% 49.8ns ± 3% -10.33% (p=0.008 n=5+5) BM_ConditionWaiters/1/8 36.2ns ± 0% 35.2ns ± 0% -2.90% (p=0.016 n=5+4) BM_ConditionWaiters/2/8 53.2ns ± 7% 48.3ns ± 7% ~ (p=0.056 n=5+5) BM_ConditionWaiters/0/64 295ns ± 1% 254ns ± 2% -13.73% (p=0.008 n=5+5) BM_ConditionWaiters/1/64 36.2ns ± 0% 35.2ns ± 0% -2.85% (p=0.008 n=5+5) BM_ConditionWaiters/2/64 290ns ± 6% 250ns ± 4% -13.68% (p=0.008 n=5+5) BM_ConditionWaiters/0/512 5.50µs ±12% 4.99µs ± 8% ~ (p=0.056 n=5+5) BM_ConditionWaiters/1/512 36.7ns ± 3% 35.2ns ± 0% -4.10% (p=0.008 n=5+5) BM_ConditionWaiters/2/512 4.44µs ±13% 4.01µs ± 3% -9.74% (p=0.008 n=5+5) BM_ConditionWaiters/0/4096 104µs ± 6% 101µs ± 3% ~ (p=0.548 n=5+5) BM_ConditionWaiters/1/4096 36.2ns ± 0% 35.1ns ± 0% -3.03% (p=0.008 n=5+5) BM_ConditionWaiters/2/4096 90.4µs ± 5% 85.3µs ± 7% ~ (p=0.222 n=5+5) BM_ConditionWaiters/0/8192 384µs ± 5% 367µs ± 7% ~ (p=0.222 n=5+5) BM_ConditionWaiters/1/8192 36.2ns ± 0% 35.2ns ± 0% -2.84% (p=0.008 n=5+5) BM_ConditionWaiters/2/8192 363µs ± 3% 316µs ± 7% -12.84% (p=0.008 n=5+5) PiperOrigin-RevId: 565669535 Change-Id: I5180c4a787933d2ce477b004a111853753304684 --- absl/synchronization/mutex.cc | 34 ++++++++---------------------- absl/synchronization/mutex.h | 4 +++- absl/synchronization/mutex_test.cc | 13 ++++++++++++ 3 files changed, 25 insertions(+), 26 deletions(-) diff --git a/absl/synchronization/mutex.cc b/absl/synchronization/mutex.cc index ea0d96cfbfe..8148fbde8a6 100644 --- a/absl/synchronization/mutex.cc +++ b/absl/synchronization/mutex.cc @@ -960,8 +960,7 @@ static PerThreadSynch* Enqueue(PerThreadSynch* head, SynchWaitParams* waitp, } while (s->priority <= advance_to->priority); // termination guaranteed because s->priority > head->priority // and head is the end of a skip chain - } else if (waitp->how == kExclusive && - Condition::GuaranteedEqual(waitp->cond, nullptr)) { + } else if (waitp->how == kExclusive && waitp->cond == nullptr) { // An unlocker could be scanning the queue, but we know it will recheck // the queue front for writers that have no condition, which is what s // is, so an insert at front is safe. @@ -1542,15 +1541,11 @@ bool Mutex::AwaitCommon(const Condition& cond, KernelTimeout t) { SynchWaitParams waitp(how, &cond, t, nullptr /*no cvmu*/, Synch_GetPerThreadAnnotated(this), nullptr /*no cv_word*/); - int flags = kMuHasBlocked; - if (!Condition::GuaranteedEqual(&cond, nullptr)) { - flags |= kMuIsCond; - } this->UnlockSlow(&waitp); this->Block(waitp.thread); ABSL_TSAN_MUTEX_POST_UNLOCK(this, TsanFlags(how)); ABSL_TSAN_MUTEX_PRE_LOCK(this, TsanFlags(how)); - this->LockSlowLoop(&waitp, flags); + this->LockSlowLoop(&waitp, kMuHasBlocked | kMuIsCond); bool res = waitp.cond != nullptr || // => cond known true from LockSlowLoop EvalConditionAnnotated(&cond, this, true, false, how == kShared); ABSL_TSAN_MUTEX_POST_LOCK(this, TsanFlags(how), 0); @@ -1834,7 +1829,7 @@ bool Mutex::LockSlowWithDeadline(MuHow how, const Condition* cond, SynchWaitParams waitp(how, cond, t, nullptr /*no cvmu*/, Synch_GetPerThreadAnnotated(this), nullptr /*no cv_word*/); - if (!Condition::GuaranteedEqual(cond, nullptr)) { + if (cond != nullptr) { flags |= kMuIsCond; } if (unlock) { @@ -2024,7 +2019,6 @@ ABSL_ATTRIBUTE_NOINLINE void Mutex::UnlockSlow(SynchWaitParams* waitp) { // head of the list searched previously, or zero PerThreadSynch* old_h = nullptr; // a condition that's known to be false. - const Condition* known_false = nullptr; PerThreadSynch* wake_list = kPerThreadSynchNull; // list of threads to wake intptr_t wr_wait = 0; // set to kMuWrWait if we wake a reader and a // later writer could have acquired the lock @@ -2128,7 +2122,7 @@ ABSL_ATTRIBUTE_NOINLINE void Mutex::UnlockSlow(SynchWaitParams* waitp) { } } if (h->next->waitp->how == kExclusive && - Condition::GuaranteedEqual(h->next->waitp->cond, nullptr)) { + h->next->waitp->cond == nullptr) { // easy case: writer with no condition; no need to search pw = h; // wake w, the successor of h (=pw) w = h->next; @@ -2211,10 +2205,8 @@ ABSL_ATTRIBUTE_NOINLINE void Mutex::UnlockSlow(SynchWaitParams* waitp) { w_walk->wake = false; if (w_walk->waitp->cond == nullptr || // no condition => vacuously true OR - (w_walk->waitp->cond != known_false && - // this thread's condition is not known false, AND - // is in fact true - EvalConditionIgnored(this, w_walk->waitp->cond))) { + // this thread's condition is true + EvalConditionIgnored(this, w_walk->waitp->cond)) { if (w == nullptr) { w_walk->wake = true; // can wake this waiter w = w_walk; @@ -2228,8 +2220,6 @@ ABSL_ATTRIBUTE_NOINLINE void Mutex::UnlockSlow(SynchWaitParams* waitp) { } else { // writer with true condition wr_wait = kMuWrWait; } - } else { // can't wake; condition false - known_false = w_walk->waitp->cond; // remember last false condition } if (w_walk->wake) { // we're waking reader w_walk pw_walk = w_walk; // don't skip similar waiters @@ -2722,17 +2712,11 @@ Condition::Condition(const bool* cond) StoreCallback(dereference); } -bool Condition::Eval() const { - // eval_ == null for kTrue - return (this->eval_ == nullptr) || (*this->eval_)(this); -} +bool Condition::Eval() const { return (*this->eval_)(this); } bool Condition::GuaranteedEqual(const Condition* a, const Condition* b) { - // kTrue logic. - if (a == nullptr || a->eval_ == nullptr) { - return b == nullptr || b->eval_ == nullptr; - } else if (b == nullptr || b->eval_ == nullptr) { - return false; + if (a == nullptr || b == nullptr) { + return a == b; } // Check equality of the representative fields. return a->eval_ == b->eval_ && a->arg_ == b->arg_ && diff --git a/absl/synchronization/mutex.h b/absl/synchronization/mutex.h index ff4747b54ac..1fb6cca1a14 100644 --- a/absl/synchronization/mutex.h +++ b/absl/synchronization/mutex.h @@ -859,8 +859,10 @@ class Condition { std::memcpy(callback, callback_, sizeof(*callback)); } + static bool AlwaysTrue(const Condition*) { return true; } + // Used only to create kTrue. - constexpr Condition() = default; + constexpr Condition() : eval_(AlwaysTrue), arg_(nullptr) {} }; // ----------------------------------------------------------------------------- diff --git a/absl/synchronization/mutex_test.cc b/absl/synchronization/mutex_test.cc index 0bca46c5a54..6c38c07c5a0 100644 --- a/absl/synchronization/mutex_test.cc +++ b/absl/synchronization/mutex_test.cc @@ -1025,6 +1025,19 @@ TEST(Mutex, FunctorCondition) { } } +TEST(Mutex, ConditionSwap) { + // Ensure that Conditions can be swap'ed. + bool b1 = true; + absl::Condition c1(&b1); + bool b2 = false; + absl::Condition c2(&b2); + EXPECT_TRUE(c1.Eval()); + EXPECT_FALSE(c2.Eval()); + std::swap(c1, c2); + EXPECT_FALSE(c1.Eval()); + EXPECT_TRUE(c2.Eval()); +} + // -------------------------------------------------------- // Test for bug with pattern of readers using a condvar. The bug was that if a // reader went to sleep on a condition variable while one or more other readers