From ffc2926d144fca7f20a11021dc42213a08498eb6 Mon Sep 17 00:00:00 2001 From: "fpizlo@apple.com" Date: Wed, 20 Apr 2016 04:25:02 +0000 Subject: [PATCH] Clean up the ParkingLot uparking API a bit https://bugs.webkit.org/show_bug.cgi?id=156746 Reviewed by Saam Barati and Geoffrey Garen. Previously, unparkOne() would either return a boolean to tell you if there are any more threads on the queue or it would pass your callback a pair of booleans - one to tell if a thread was unparked and another to tell if there are any more threads. This was an annoying inconsistency. What if you wanted to know if unparkOne() unparked a thread but you don't care to use callbacks? This fixes unparkOne() to use a struct called UnparkResult for both of its variants. This makes the code a bit cleaner. * wtf/Atomics.h: Add some more atomic ops. (WTF::Atomic::exchangeAndAdd): (WTF::Atomic::exchange): * wtf/Condition.h: Change calls to unparkOne(). (WTF::ConditionBase::notifyOne): * wtf/Lock.cpp: Change calls to unparkOne(). (WTF::LockBase::unlockSlow): * wtf/ParkingLot.cpp: (WTF::ParkingLot::parkConditionally): (WTF::ParkingLot::unparkOne): * wtf/ParkingLot.h: Switch to using ScopedLambda and introduce UnparkResult. git-svn-id: https://svn.webkit.org/repository/webkit/trunk@199760 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Source/WTF/ChangeLog | 27 +++++++++++++++++++++++ Source/WTF/wtf/Atomics.h | 19 ++++++++++++++++ Source/WTF/wtf/Condition.h | 4 ++-- Source/WTF/wtf/Lock.cpp | 4 ++-- Source/WTF/wtf/ParkingLot.cpp | 42 ++++++++++++++++++++--------------- Source/WTF/wtf/ParkingLot.h | 51 ++++++++++++++++++++++++++++++++++--------- 6 files changed, 115 insertions(+), 32 deletions(-) diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog index aeae86b..b775dc0 100644 --- a/Source/WTF/ChangeLog +++ b/Source/WTF/ChangeLog @@ -1,3 +1,30 @@ +2016-04-19 Filip Pizlo + + Clean up the ParkingLot uparking API a bit + https://bugs.webkit.org/show_bug.cgi?id=156746 + + Reviewed by Saam Barati and Geoffrey Garen. + + Previously, unparkOne() would either return a boolean to tell you if there are any more threads on + the queue or it would pass your callback a pair of booleans - one to tell if a thread was unparked + and another to tell if there are any more threads. This was an annoying inconsistency. What if you + wanted to know if unparkOne() unparked a thread but you don't care to use callbacks? + + This fixes unparkOne() to use a struct called UnparkResult for both of its variants. This makes the + code a bit cleaner. + + * wtf/Atomics.h: Add some more atomic ops. + (WTF::Atomic::exchangeAndAdd): + (WTF::Atomic::exchange): + * wtf/Condition.h: Change calls to unparkOne(). + (WTF::ConditionBase::notifyOne): + * wtf/Lock.cpp: Change calls to unparkOne(). + (WTF::LockBase::unlockSlow): + * wtf/ParkingLot.cpp: + (WTF::ParkingLot::parkConditionally): + (WTF::ParkingLot::unparkOne): + * wtf/ParkingLot.h: Switch to using ScopedLambda and introduce UnparkResult. + 2016-04-19 Commit Queue Unreviewed, rolling out r199726. diff --git a/Source/WTF/wtf/Atomics.h b/Source/WTF/wtf/Atomics.h index ed7d4eb..ce4d5da 100644 --- a/Source/WTF/wtf/Atomics.h +++ b/Source/WTF/wtf/Atomics.h @@ -76,6 +76,25 @@ struct Atomic { T expectedOrActual = expected; return value.compare_exchange_strong(expectedOrActual, desired, order); } + + template + T exchangeAndAdd(U addend, std::memory_order order = std::memory_order_seq_cst) + { +#if OS(WINDOWS) + // See above. + order = std::memory_order_seq_cst; +#endif + return value.fetch_add(addend, order); + } + + T exchange(T newValue, std::memory_order order = std::memory_order_seq_cst) + { +#if OS(WINDOWS) + // See above. + order = std::memory_order_seq_cst; +#endif + return value.exchange(newValue, order); + } std::atomic value; }; diff --git a/Source/WTF/wtf/Condition.h b/Source/WTF/wtf/Condition.h index b811372..ff71ba5 100644 --- a/Source/WTF/wtf/Condition.h +++ b/Source/WTF/wtf/Condition.h @@ -180,8 +180,8 @@ struct ConditionBase { ParkingLot::unparkOne( &m_hasWaiters, - [this] (bool, bool mayHaveMoreThreads) { - if (!mayHaveMoreThreads) + [this] (ParkingLot::UnparkResult result) { + if (!result.mayHaveMoreThreads) m_hasWaiters.store(false); }); } diff --git a/Source/WTF/wtf/Lock.cpp b/Source/WTF/wtf/Lock.cpp index cb99042..d7510a6 100644 --- a/Source/WTF/wtf/Lock.cpp +++ b/Source/WTF/wtf/Lock.cpp @@ -94,12 +94,12 @@ NEVER_INLINE void LockBase::unlockSlow() ASSERT(oldByteValue == (isHeldBit | hasParkedBit)); ParkingLot::unparkOne( &m_byte, - [this] (bool, bool mayHaveMoreThreads) { + [this] (ParkingLot::UnparkResult result) { // We are the only ones that can clear either the isHeldBit or the hasParkedBit, // so we should still see both bits set right now. ASSERT(m_byte.load() == (isHeldBit | hasParkedBit)); - if (mayHaveMoreThreads) + if (result.mayHaveMoreThreads) m_byte.store(hasParkedBit); else m_byte.store(0); diff --git a/Source/WTF/wtf/ParkingLot.cpp b/Source/WTF/wtf/ParkingLot.cpp index 62db4be..027c727 100644 --- a/Source/WTF/wtf/ParkingLot.cpp +++ b/Source/WTF/wtf/ParkingLot.cpp @@ -530,10 +530,10 @@ bool dequeue( } // anonymous namespace -NEVER_INLINE bool ParkingLot::parkConditionally( +NEVER_INLINE bool ParkingLot::parkConditionallyImpl( const void* address, - std::function validation, - std::function beforeSleep, + const ScopedLambda& validation, + const ScopedLambda& beforeSleep, Clock::time_point timeout) { if (verbose) @@ -591,14 +591,11 @@ NEVER_INLINE bool ParkingLot::parkConditionally( // probably worthwhile to detect when this happens, and return true in that case, to ensure // that when we return false it really means that no unpark could have been responsible for us // waking up, and that if an unpark call did happen, it woke someone else up. - bool didFind = false; dequeue( address, BucketMode::IgnoreEmpty, [&] (ThreadData* element) { - if (element == me) { - didFind = true; + if (element == me) return DequeueResult::RemoveAndStop; - } return DequeueResult::Ignore; }, [] (bool) { }); @@ -612,29 +609,35 @@ NEVER_INLINE bool ParkingLot::parkConditionally( } // If we were not found in the search above, then we know that someone unparked us. - return !didFind; + return false; } -NEVER_INLINE bool ParkingLot::unparkOne(const void* address) +NEVER_INLINE ParkingLot::UnparkResult ParkingLot::unparkOne(const void* address) { if (verbose) dataLog(toString(currentThread(), ": unparking one.\n")); + + UnparkResult result; ThreadData* threadData = nullptr; - bool result = dequeue( + result.mayHaveMoreThreads = dequeue( address, - BucketMode::IgnoreEmpty, + BucketMode::EnsureNonEmpty, [&] (ThreadData* element) { if (element->address != address) return DequeueResult::Ignore; threadData = element; + result.didUnparkThread = true; return DequeueResult::RemoveAndStop; }, [] (bool) { }); - if (!threadData) - return false; - + if (!threadData) { + ASSERT(!result.didUnparkThread); + result.mayHaveMoreThreads = false; + return result; + } + ASSERT(threadData->address); { @@ -646,9 +649,9 @@ NEVER_INLINE bool ParkingLot::unparkOne(const void* address) return result; } -NEVER_INLINE void ParkingLot::unparkOne( +NEVER_INLINE void ParkingLot::unparkOneImpl( const void* address, - std::function callback) + const ScopedLambda& callback) { if (verbose) dataLog(toString(currentThread(), ": unparking one the hard way.\n")); @@ -664,7 +667,10 @@ NEVER_INLINE void ParkingLot::unparkOne( return DequeueResult::RemoveAndStop; }, [&] (bool mayHaveMoreThreads) { - callback(!!threadData, threadData && mayHaveMoreThreads); + UnparkResult result; + result.didUnparkThread = !!threadData; + result.mayHaveMoreThreads = result.didUnparkThread && mayHaveMoreThreads; + callback(result); }); if (!threadData) @@ -713,7 +719,7 @@ NEVER_INLINE void ParkingLot::unparkAll(const void* address) dataLog(toString(currentThread(), ": done unparking.\n")); } -NEVER_INLINE void ParkingLot::forEach(std::function callback) +NEVER_INLINE void ParkingLot::forEachImpl(const ScopedLambda& callback) { Vector bucketsToUnlock = lockHashtable(); diff --git a/Source/WTF/wtf/ParkingLot.h b/Source/WTF/wtf/ParkingLot.h index 747c116..ccfdb9c 100644 --- a/Source/WTF/wtf/ParkingLot.h +++ b/Source/WTF/wtf/ParkingLot.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2015 Apple Inc. All rights reserved. + * Copyright (C) 2015-2016 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -29,6 +29,7 @@ #include #include #include +#include #include namespace WTF { @@ -51,11 +52,19 @@ public: // you don't recursively call parkConditionally(). You can call unparkOne()/unparkAll() though. // It's useful to use beforeSleep() to unlock some mutex in the implementation of // Condition::wait(). - WTF_EXPORT_PRIVATE static bool parkConditionally( + template + static bool parkConditionally( const void* address, - std::function validation, - std::function beforeSleep, - Clock::time_point timeout); + ValidationFunctor&& validation, + BeforeSleepFunctor&& beforeSleep, + Clock::time_point timeout) + { + return parkConditionallyImpl( + address, + scopedLambda(std::forward(validation)), + scopedLambda(std::forward(beforeSleep)), + timeout); + } // Simple version of parkConditionally() that covers the most common case: you want to park // indefinitely so long as the value at the given address hasn't changed. @@ -75,7 +84,11 @@ public: // Unparks one thread from the queue associated with the given address, which cannot be null. // Returns true if there may still be other threads on that queue, or false if there definitely // are no more threads on the queue. - WTF_EXPORT_PRIVATE static bool unparkOne(const void* address); + struct UnparkResult { + bool didUnparkThread { false }; + bool mayHaveMoreThreads { false }; + }; + WTF_EXPORT_PRIVATE static UnparkResult unparkOne(const void* address); // Unparks one thread from the queue associated with the given address, and calls the given // functor while the address is locked. Reports to the callback whether any thread got unparked @@ -88,9 +101,11 @@ public: // that race - see Rusty Russel's well-known usersem library - but it's not pretty. This form // allows that race to be completely avoided, since there is no way that a thread can be parked // while the callback is running. - WTF_EXPORT_PRIVATE static void unparkOne( - const void* address, - std::function callback); + template + static void unparkOne(const void* address, CallbackFunctor&& callback) + { + unparkOneImpl(address, scopedLambda(std::forward(callback))); + } // Unparks every thread from the queue associated with the given address, which cannot be null. WTF_EXPORT_PRIVATE static void unparkAll(const void* address); @@ -108,7 +123,23 @@ public: // As well as many other possible interleavings that all have T1 before T2 and T3 before T4 but are // otherwise unconstrained. This method is useful primarily for debugging. It's also used by unit // tests. - WTF_EXPORT_PRIVATE static void forEach(std::function); + template + static void forEach(CallbackFunctor&& callback) + { + forEachImpl(scopedLambda(std::forward(callback))); + } + +private: + WTF_EXPORT_PRIVATE static bool parkConditionallyImpl( + const void* address, + const ScopedLambda& validation, + const ScopedLambda& beforeSleep, + Clock::time_point timeout); + + WTF_EXPORT_PRIVATE static void unparkOneImpl( + const void* address, const ScopedLambda& callback); + + WTF_EXPORT_PRIVATE static void forEachImpl(const ScopedLambda&); }; } // namespace WTF -- 1.8.3.1