Clean up the ParkingLot uparking API a bit
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Apr 2016 04:25:02 +0000 (04:25 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Apr 2016 04:25:02 +0000 (04:25 +0000)
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
Source/WTF/wtf/Atomics.h
Source/WTF/wtf/Condition.h
Source/WTF/wtf/Lock.cpp
Source/WTF/wtf/ParkingLot.cpp
Source/WTF/wtf/ParkingLot.h

index aeae86b..b775dc0 100644 (file)
@@ -1,3 +1,30 @@
+2016-04-19  Filip Pizlo  <fpizlo@apple.com>
+
+        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  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r199726.
index ed7d4eb..ce4d5da 100644 (file)
@@ -76,6 +76,25 @@ struct Atomic {
         T expectedOrActual = expected;
         return value.compare_exchange_strong(expectedOrActual, desired, order);
     }
+    
+    template<typename U>
+    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<T> value;
 };
index b811372..ff71ba5 100644 (file)
@@ -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);
             });
     }
index cb99042..d7510a6 100644 (file)
@@ -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);
index 62db4be..027c727 100644 (file)
@@ -530,10 +530,10 @@ bool dequeue(
 
 } // anonymous namespace
 
-NEVER_INLINE bool ParkingLot::parkConditionally(
+NEVER_INLINE bool ParkingLot::parkConditionallyImpl(
     const void* address,
-    std::function<bool()> validation,
-    std::function<void()> beforeSleep,
+    const ScopedLambda<bool()>& validation,
+    const ScopedLambda<void()>& 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<void(bool didUnparkThread, bool mayHaveMoreThreads)> callback)
+    const ScopedLambda<void(ParkingLot::UnparkResult)>& 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<void(ThreadIdentifier, const void*)> callback)
+NEVER_INLINE void ParkingLot::forEachImpl(const ScopedLambda<void(ThreadIdentifier, const void*)>& callback)
 {
     Vector<Bucket*> bucketsToUnlock = lockHashtable();
 
index 747c116..ccfdb9c 100644 (file)
@@ -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 <chrono>
 #include <functional>
 #include <wtf/Atomics.h>
+#include <wtf/ScopedLambda.h>
 #include <wtf/Threading.h>
 
 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<typename ValidationFunctor, typename BeforeSleepFunctor>
+    static bool parkConditionally(
         const void* address,
-        std::function<bool()> validation,
-        std::function<void()> beforeSleep,
-        Clock::time_point timeout);
+        ValidationFunctor&& validation,
+        BeforeSleepFunctor&& beforeSleep,
+        Clock::time_point timeout)
+    {
+        return parkConditionallyImpl(
+            address,
+            scopedLambda<bool()>(std::forward<ValidationFunctor>(validation)),
+            scopedLambda<void()>(std::forward<BeforeSleepFunctor>(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<void(bool didUnparkThread, bool mayHaveMoreThreads)> callback);
+    template<typename CallbackFunctor>
+    static void unparkOne(const void* address, CallbackFunctor&& callback)
+    {
+        unparkOneImpl(address, scopedLambda<void(UnparkResult)>(std::forward<CallbackFunctor>(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<void(ThreadIdentifier, const void*)>);
+    template<typename CallbackFunctor>
+    static void forEach(CallbackFunctor&& callback)
+    {
+        forEachImpl(scopedLambda<void(ThreadIdentifier, const void*)>(std::forward<CallbackFunctor>(callback)));
+    }
+
+private:
+    WTF_EXPORT_PRIVATE static bool parkConditionallyImpl(
+        const void* address,
+        const ScopedLambda<bool()>& validation,
+        const ScopedLambda<void()>& beforeSleep,
+        Clock::time_point timeout);
+    
+    WTF_EXPORT_PRIVATE static void unparkOneImpl(
+        const void* address, const ScopedLambda<void(UnparkResult)>& callback);
+
+    WTF_EXPORT_PRIVATE static void forEachImpl(const ScopedLambda<void(ThreadIdentifier, const void*)>&);
 };
 
 } // namespace WTF