It should be easy to decide how WebKit yields

https://bugs.webkit.org/show_bug.cgi?id=174298 Reviewed by Saam Barati. Source/bmalloc: Use sched_yield() explicitly. * bmalloc/StaticMutex.cpp: (bmalloc::StaticMutex::lockSlowCase): Source/JavaScriptCore: Use the new WTF::Thread::yield() function for yielding instead of the C++ function. * heap/Heap.cpp: (JSC::Heap::resumeThePeriphery): * heap/VisitingTimeout.h: * runtime/JSCell.cpp: (JSC::JSCell::lockSlow): (JSC::JSCell::unlockSlow): * runtime/JSCell.h: * runtime/JSCellInlines.h: (JSC::JSCell::lock): (JSC::JSCell::unlock): * runtime/JSLock.cpp: (JSC::JSLock::grabAllLocks): * runtime/SamplingProfiler.cpp: Source/WebCore: No new tests because the WebCore change is just a change to how we #include things. * inspector/InspectorPageAgent.h: * inspector/TimelineRecordFactory.h: * workers/Worker.h: * workers/WorkerGlobalScopeProxy.h: * workers/WorkerMessagingProxy.h: Source/WebKitLegacy: * Storage/StorageTracker.h: Source/WTF: Created a Thread::yield() abstraction for sched_yield(), and made WTF use it everywhere that it had previously used std::this_thread::yield(). To make it less annoying to experiment with changes to the lock algorithm in the future, this also moves the meat of the algorithm into LockAlgorithmInlines.h. Only two files include that header. Since LockAlgorithm.h no longer includes ParkingLot.h, a bunch of files in WK now need to include timing headers (Seconds, MonotonicTime, etc) manually. * WTF.xcodeproj/project.pbxproj: * benchmarks/ToyLocks.h: * wtf/CMakeLists.txt: * wtf/Lock.cpp: * wtf/LockAlgorithm.h: (WTF::LockAlgorithm::lockSlow): Deleted. (WTF::LockAlgorithm::unlockSlow): Deleted. * wtf/LockAlgorithmInlines.h: Added. (WTF::hasParkedBit>::lockSlow): (WTF::hasParkedBit>::unlockSlow): * wtf/MainThread.cpp: * wtf/RunLoopTimer.h: * wtf/Threading.cpp: * wtf/Threading.h: * wtf/ThreadingPthreads.cpp: (WTF::Thread::yield): * wtf/ThreadingWin.cpp: (WTF::Thread::yield): * wtf/WordLock.cpp: (WTF::WordLockBase::lockSlow): (WTF::WordLockBase::unlockSlow): Canonical link: https://commits.webkit.org/191572@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219763 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jul 22, 2017 · db0f4c3 · db0f4c3
1 parent 2e280e9
commit db0f4c3
Show file tree

Hide file tree

Showing 32 changed files with 310 additions and 124 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,26 @@
+2017-07-14  Filip Pizlo  <fpizlo@apple.com>
+
+        It should be easy to decide how WebKit yields
+        https://bugs.webkit.org/show_bug.cgi?id=174298
+
+        Reviewed by Saam Barati.
+
+        Use the new WTF::Thread::yield() function for yielding instead of the C++ function.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::resumeThePeriphery):
+        * heap/VisitingTimeout.h:
+        * runtime/JSCell.cpp:
+        (JSC::JSCell::lockSlow):
+        (JSC::JSCell::unlockSlow):
+        * runtime/JSCell.h:
+        * runtime/JSCellInlines.h:
+        (JSC::JSCell::lock):
+        (JSC::JSCell::unlock):
+        * runtime/JSLock.cpp:
+        (JSC::JSLock::grabAllLocks):
+        * runtime/SamplingProfiler.cpp:
+
 2017-07-21  Mark Lam  <mark.lam@apple.com>
 
         Refactor MASM probe CPUState to use arrays for register storage.

diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
@@ -79,6 +79,7 @@
 #include <wtf/ProcessID.h>
 #include <wtf/RAMSize.h>
 #include <wtf/SimpleStats.h>
+#include <wtf/Threading.h>
 
 #if USE(FOUNDATION)
 #if __has_include(<objc/objc-internal.h>)
@@ -1620,7 +1621,7 @@ NEVER_INLINE void Heap::resumeThePeriphery()
                 slotVisitorsToUpdate.takeLast();
             }
         }
-        std::this_thread::yield();
+        WTF::Thread::yield();
     }
 
     for (SlotVisitor* slotVisitor : slotVisitorsToUpdate)

diff --git a/Source/JavaScriptCore/heap/VisitingTimeout.h b/Source/JavaScriptCore/heap/VisitingTimeout.h
@@ -26,7 +26,7 @@
 #pragma once
 
 #include "SlotVisitor.h"
-#include <wtf/MonotonicTime.h>
+#include <wtf/TimeWithDynamicClockType.h>
 
 namespace JSC {
 

diff --git a/Source/JavaScriptCore/runtime/JSCell.cpp b/Source/JavaScriptCore/runtime/JSCell.cpp
@@ -30,6 +30,7 @@
 #include "JSObject.h"
 #include "NumberObject.h"
 #include "WebAssemblyToJSCallee.h"
+#include <wtf/LockAlgorithmInlines.h>
 #include <wtf/MathExtras.h>
 
 namespace JSC {
@@ -294,4 +295,16 @@ JSValue JSCell::getPrototype(JSObject*, ExecState*)
     RELEASE_ASSERT_NOT_REACHED();
 }
 
+void JSCell::lockSlow()
+{
+    Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
+    IndexingTypeLockAlgorithm::lockSlow(*lock);
+}
+
+void JSCell::unlockSlow()
+{
+    Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
+    IndexingTypeLockAlgorithm::unlockSlow(*lock);
+}
+
 } // namespace JSC
diff --git a/Source/JavaScriptCore/runtime/JSCell.h b/Source/JavaScriptCore/runtime/JSCell.h
@@ -265,6 +265,8 @@ class JSCell : public HeapCell {
     friend class LLIntOffsetsExtractor;
 
     JS_EXPORT_PRIVATE JSObject* toObjectSlow(ExecState*, JSGlobalObject*) const;
+    JS_EXPORT_PRIVATE void lockSlow();
+    JS_EXPORT_PRIVATE void unlockSlow();
 
     StructureID m_structureID;
     IndexingType m_indexingTypeAndMisc; // DO NOT store to this field. Always CAS.

diff --git a/Source/JavaScriptCore/runtime/JSCellInlines.h b/Source/JavaScriptCore/runtime/JSCellInlines.h
@@ -329,7 +329,8 @@ inline void JSCell::callDestructor(VM& vm)
 inline void JSCell::lock()
 {
     Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
-    IndexingTypeLockAlgorithm::lock(*lock);
+    if (UNLIKELY(!IndexingTypeLockAlgorithm::lockFast(*lock)))
+        lockSlow();
 }
 
 inline bool JSCell::tryLock()
@@ -341,7 +342,8 @@ inline bool JSCell::tryLock()
 inline void JSCell::unlock()
 {
     Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
-    IndexingTypeLockAlgorithm::unlock(*lock);
+    if (UNLIKELY(!IndexingTypeLockAlgorithm::unlockFast(*lock)))
+        unlockSlow();
 }
 
 inline bool JSCell::isLocked() const

diff --git a/Source/JavaScriptCore/runtime/JSLock.cpp b/Source/JavaScriptCore/runtime/JSLock.cpp
@@ -246,7 +246,7 @@ void JSLock::grabAllLocks(DropAllLocks* dropper, unsigned droppedLockCount)
 
     while (dropper->dropDepth() != m_lockDropDepth) {
         unlock(droppedLockCount);
-        std::this_thread::yield();
+        Thread::yield();
         lock(droppedLockCount);
     }
 

diff --git a/Source/JavaScriptCore/runtime/SamplingProfiler.cpp b/Source/JavaScriptCore/runtime/SamplingProfiler.cpp
@@ -48,6 +48,7 @@
 #include "SlotVisitor.h"
 #include "StrongInlines.h"
 #include "VM.h"
+#include <thread>
 #include <wtf/FilePrintStream.h>
 #include <wtf/HashSet.h>
 #include <wtf/RefPtr.h>

diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
@@ -1,3 +1,40 @@
+2017-07-14  Filip Pizlo  <fpizlo@apple.com>
+
+        It should be easy to decide how WebKit yields
+        https://bugs.webkit.org/show_bug.cgi?id=174298
+
+        Reviewed by Saam Barati.
+
+        Created a Thread::yield() abstraction for sched_yield(), and made WTF use it everywhere that it
+        had previously used std::this_thread::yield().
+
+        To make it less annoying to experiment with changes to the lock algorithm in the future, this also
+        moves the meat of the algorithm into LockAlgorithmInlines.h. Only two files include that header.
+        Since LockAlgorithm.h no longer includes ParkingLot.h, a bunch of files in WK now need to include
+        timing headers (Seconds, MonotonicTime, etc) manually.
+
+        * WTF.xcodeproj/project.pbxproj:
+        * benchmarks/ToyLocks.h:
+        * wtf/CMakeLists.txt:
+        * wtf/Lock.cpp:
+        * wtf/LockAlgorithm.h:
+        (WTF::LockAlgorithm::lockSlow): Deleted.
+        (WTF::LockAlgorithm::unlockSlow): Deleted.
+        * wtf/LockAlgorithmInlines.h: Added.
+        (WTF::hasParkedBit>::lockSlow):
+        (WTF::hasParkedBit>::unlockSlow):
+        * wtf/MainThread.cpp:
+        * wtf/RunLoopTimer.h:
+        * wtf/Threading.cpp:
+        * wtf/Threading.h:
+        * wtf/ThreadingPthreads.cpp:
+        (WTF::Thread::yield):
+        * wtf/ThreadingWin.cpp:
+        (WTF::Thread::yield):
+        * wtf/WordLock.cpp:
+        (WTF::WordLockBase::lockSlow):
+        (WTF::WordLockBase::unlockSlow):
+
 2017-07-22  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [WTF] Extend ThreadGroup::add results from bool to ThreadGroupAddResult

diff --git a/Source/WTF/WTF.xcodeproj/project.pbxproj b/Source/WTF/WTF.xcodeproj/project.pbxproj
@@ -174,6 +174,7 @@
 		0F30BA8D1E78708E002CA847 /* LoggingHashMap.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LoggingHashMap.h; sourceTree = "<group>"; };
 		0F30BA8E1E78708E002CA847 /* LoggingHashSet.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LoggingHashSet.h; sourceTree = "<group>"; };
 		0F30BA8F1E78708E002CA847 /* LoggingHashTraits.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LoggingHashTraits.h; sourceTree = "<group>"; };
+		0F31DD701F1308BC0072EB4A /* LockAlgorithmInlines.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = LockAlgorithmInlines.h; sourceTree = "<group>"; };
 		0F3501631BB258C800F0A2A3 /* WeakRandom.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WeakRandom.h; sourceTree = "<group>"; };
 		0F43D8EF1DB5ADDC00108FB6 /* AutomaticThread.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = AutomaticThread.cpp; sourceTree = "<group>"; };
 		0F43D8F01DB5ADDC00108FB6 /* AutomaticThread.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = AutomaticThread.h; sourceTree = "<group>"; };
@@ -343,8 +344,8 @@
 		9BC70F04176C379D00101DEC /* AtomicStringTable.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = AtomicStringTable.cpp; sourceTree = "<group>"; };
 		9BD8F40A176C2AD80002D865 /* AtomicStringTable.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = AtomicStringTable.h; sourceTree = "<group>"; };
 		9C67C542589348E285B49699 /* IndexedContainerIterator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = IndexedContainerIterator.h; sourceTree = "<group>"; };
-		A30D412C1F0DE0BA00B71954 /* SoftLinking.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = SoftLinking.h; path = SoftLinking.h; sourceTree = "<group>"; };
-		A30D412D1F0DE13F00B71954 /* SoftLinking.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = SoftLinking.h; path = SoftLinking.h; sourceTree = "<group>"; };
+		A30D412C1F0DE0BA00B71954 /* SoftLinking.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SoftLinking.h; sourceTree = "<group>"; };
+		A30D412D1F0DE13F00B71954 /* SoftLinking.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SoftLinking.h; sourceTree = "<group>"; };
 		A5098AFF1C169E0700087797 /* SandboxSPI.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SandboxSPI.h; sourceTree = "<group>"; };
 		A5098B011C16A4F900087797 /* SecuritySPI.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SecuritySPI.h; sourceTree = "<group>"; };
 		A561F30F1DF2642100FF675D /* DeprecatedOptional.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DeprecatedOptional.h; sourceTree = "<group>"; };
@@ -829,6 +830,7 @@
 				0FF4B4C41E88939C00DBBE86 /* Liveness.h */,
 				0FE164681B6FFC9600400E7C /* Lock.cpp */,
 				0FE164691B6FFC9600400E7C /* Lock.h */,
+				0F31DD701F1308BC0072EB4A /* LockAlgorithmInlines.h */,
 				0F0FCDDD1DD167F900CCAB53 /* LockAlgorithm.h */,
 				0F60F32D1DFCBD1B00416D6C /* LockedPrintStream.cpp */,
 				0F60F32E1DFCBD1B00416D6C /* LockedPrintStream.h */,

diff --git a/Source/WTF/benchmarks/ToyLocks.h b/Source/WTF/benchmarks/ToyLocks.h
@@ -52,7 +52,7 @@ class YieldSpinLock {
     void lock()
     {
         while (!m_lock.compareExchangeWeak(0, 1, std::memory_order_acquire))
-            std::this_thread::yield();
+            Thread::yield();
     }
 
     void unlock()
@@ -118,7 +118,7 @@ class TransactionalSpinLock {
                 : "memory");
             if (result)
                 return;
-            std::this_thread::yield();
+            Thread::yield();
         }
     }
 
@@ -215,7 +215,7 @@ class BargingLock {
             if (currentState & hasParkedBit)
                 break;
 
-            std::this_thread::yield();
+            Thread::yield();
         }
 
         for (;;) {
@@ -292,7 +292,7 @@ class ThunderLock {
             if (currentState == LockedAndParked)
                 break;
 
-            std::this_thread::yield();
+            Thread::yield();
         }
 
         for (;;) {
@@ -361,7 +361,7 @@ class CascadeLock {
             if (currentState == LockedAndParked)
                 break;
 
-            std::this_thread::yield();
+            Thread::yield();
         }
 
         State desiredState = Locked;

diff --git a/Source/WTF/wtf/CMakeLists.txt b/Source/WTF/wtf/CMakeLists.txt
@@ -60,6 +60,7 @@ set(WTF_HEADERS
     ListHashSet.h
     Liveness.h
     Lock.h
+    LockAlgorithmInlines.h
     LockAlgorithm.h
     LockedPrintStream.h
     Locker.h

diff --git a/Source/WTF/wtf/Lock.cpp b/Source/WTF/wtf/Lock.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -26,6 +26,8 @@
 #include "config.h"
 #include "Lock.h"
 
+#include <wtf/LockAlgorithmInlines.h>
+
 namespace WTF {
 
 void LockBase::lockSlow()

diff --git a/Source/WTF/wtf/LockAlgorithm.h b/Source/WTF/wtf/LockAlgorithm.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -26,10 +26,9 @@
 #ifndef WTF_LockAlgorithm_h
 #define WTF_LockAlgorithm_h
 
-#include <thread>
 #include <wtf/Atomics.h>
 #include <wtf/Compiler.h>
-#include <wtf/ParkingLot.h>
+#include <wtf/Threading.h>
 
 namespace WTF {
 
@@ -123,108 +122,13 @@ class LockAlgorithm {
         return lock.load(std::memory_order_acquire) & isHeldBit;
     }
 
-    NEVER_INLINE static void lockSlow(Atomic<LockType>& lock)
-    {
-        unsigned spinCount = 0;
-
-        // This magic number turns out to be optimal based on past JikesRVM experiments.
-        const unsigned spinLimit = 40;
-
-        for (;;) {
-            uint8_t currentByteValue = lock.load();
-
-            // We allow ourselves to barge in.
-            if (!(currentByteValue & isHeldBit)
-                && lock.compareExchangeWeak(currentByteValue, currentByteValue | isHeldBit))
-                return;
-
-            // If there is nobody parked and we haven't spun too much, we can just try to spin around.
-            if (!(currentByteValue & hasParkedBit) && spinCount < spinLimit) {
-                spinCount++;
-                std::this_thread::yield();
-                continue;
-            }
-
-            // Need to park. We do this by setting the parked bit first, and then parking. We spin around
-            // if the parked bit wasn't set and we failed at setting it.
-            if (!(currentByteValue & hasParkedBit)
-                && !lock.compareExchangeWeak(currentByteValue, currentByteValue | hasParkedBit))
-                continue;
-
-            // We now expect the value to be isHeld|hasParked. So long as that's the case, we can park.
-            ParkingLot::ParkResult parkResult =
-                ParkingLot::compareAndPark(&lock, currentByteValue | isHeldBit | hasParkedBit);
-            if (parkResult.wasUnparked) {
-                switch (static_cast<Token>(parkResult.token)) {
-                case DirectHandoff:
-                    // The lock was never released. It was handed to us directly by the thread that did
-                    // unlock(). This means we're done!
-                    RELEASE_ASSERT(isLocked(lock));
-                    return;
-                case BargingOpportunity:
-                    // This is the common case. The thread that called unlock() has released the lock,
-                    // and we have been woken up so that we may get an opportunity to grab the lock. But
-                    // other threads may barge, so the best that we can do is loop around and try again.
-                    break;
-                }
-            }
-
-            // We have awoken, or we never parked because the byte value changed. Either way, we loop
-            // around and try again.
-        }
-    }
+    NEVER_INLINE static void lockSlow(Atomic<LockType>& lock);
 
     enum Fairness {
-        Fair,
-        Unfair
+        Unfair,
+        Fair
     };
-    NEVER_INLINE static void unlockSlow(Atomic<LockType>& lock, Fairness fairness)
-    {
-        // We could get here because the weak CAS in unlock() failed spuriously, or because there is
-        // someone parked. So, we need a CAS loop: even if right now the lock is just held, it could
-        // be held and parked if someone attempts to lock just as we are unlocking.
-        for (;;) {
-            uint8_t oldByteValue = lock.load();
-            RELEASE_ASSERT(
-                (oldByteValue & mask) == isHeldBit
-                || (oldByteValue & mask) == (isHeldBit | hasParkedBit));
-
-            if ((oldByteValue & mask) == isHeldBit) {
-                if (lock.compareExchangeWeak(oldByteValue, oldByteValue & ~isHeldBit))
-                    return;
-                continue;
-            }
-
-            // Someone is parked. Unpark exactly one thread. We may hand the lock to that thread
-            // directly, or we will unlock the lock at the same time as we unpark to allow for barging.
-            // When we unlock, we may leave the parked bit set if there is a chance that there are still
-            // other threads parked.
-            ASSERT((oldByteValue & mask) == (isHeldBit | hasParkedBit));
-            ParkingLot::unparkOne(
-                &lock,
-                [&] (ParkingLot::UnparkResult result) -> intptr_t {
-                    // 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((lock.load() & mask) == (isHeldBit | hasParkedBit));
-
-                    if (result.didUnparkThread && (fairness == Fair || result.timeToBeFair)) {
-                        // We don't unlock anything. Instead, we hand the lock to the thread that was
-                        // waiting.
-                        return DirectHandoff;
-                    }
-
-                    lock.transaction(
-                        [&] (LockType& value) -> bool {
-                            value &= ~mask;
-                            if (result.mayHaveMoreThreads)
-                                value |= hasParkedBit;
-                            return true;
-                        });
-                    return BargingOpportunity;
-                });
-            return;
-        }
-    }
+    NEVER_INLINE static void unlockSlow(Atomic<LockType>& lock, Fairness fairness = Unfair);
 
     NEVER_INLINE static void safepointSlow(Atomic<LockType>& lockWord)
     {