Diff
Modified: trunk/Source/WTF/ChangeLog (199759 => 199760)
--- trunk/Source/WTF/ChangeLog 2016-04-20 04:14:02 UTC (rev 199759)
+++ trunk/Source/WTF/ChangeLog 2016-04-20 04:25:02 UTC (rev 199760)
@@ -1,3 +1,30 @@
+2016-04-19 Filip Pizlo <fpi...@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-qu...@webkit.org>
Unreviewed, rolling out r199726.
Modified: trunk/Source/WTF/wtf/Atomics.h (199759 => 199760)
--- trunk/Source/WTF/wtf/Atomics.h 2016-04-20 04:14:02 UTC (rev 199759)
+++ trunk/Source/WTF/wtf/Atomics.h 2016-04-20 04:25:02 UTC (rev 199760)
@@ -76,6 +76,25 @@
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;
};
Modified: trunk/Source/WTF/wtf/Condition.h (199759 => 199760)
--- trunk/Source/WTF/wtf/Condition.h 2016-04-20 04:14:02 UTC (rev 199759)
+++ trunk/Source/WTF/wtf/Condition.h 2016-04-20 04:25:02 UTC (rev 199760)
@@ -180,8 +180,8 @@
ParkingLot::unparkOne(
&m_hasWaiters,
- [this] (bool, bool mayHaveMoreThreads) {
- if (!mayHaveMoreThreads)
+ [this] (ParkingLot::UnparkResult result) {
+ if (!result.mayHaveMoreThreads)
m_hasWaiters.store(false);
});
}
Modified: trunk/Source/WTF/wtf/Lock.cpp (199759 => 199760)
--- trunk/Source/WTF/wtf/Lock.cpp 2016-04-20 04:14:02 UTC (rev 199759)
+++ trunk/Source/WTF/wtf/Lock.cpp 2016-04-20 04:25:02 UTC (rev 199760)
@@ -94,12 +94,12 @@
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);
Modified: trunk/Source/WTF/wtf/ParkingLot.cpp (199759 => 199760)
--- trunk/Source/WTF/wtf/ParkingLot.cpp 2016-04-20 04:14:02 UTC (rev 199759)
+++ trunk/Source/WTF/wtf/ParkingLot.cpp 2016-04-20 04:25:02 UTC (rev 199760)
@@ -530,10 +530,10 @@
} // 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 @@
// 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 @@
}
// 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 @@
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 @@
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 @@
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();
Modified: trunk/Source/WTF/wtf/ParkingLot.h (199759 => 199760)
--- trunk/Source/WTF/wtf/ParkingLot.h 2016-04-20 04:14:02 UTC (rev 199759)
+++ trunk/Source/WTF/wtf/ParkingLot.h 2016-04-20 04:25:02 UTC (rev 199760)
@@ -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 @@
// 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 @@
// 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 @@
// 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 @@
// 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
