Attachment 339097 Details for Bug 185121 – Patch

[patch] Patch

bug-185121-20180430034031.patch (text/plain), 17.29 KB, created by Yusuke Suzuki on 2018-04-29 11:40:32 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2018-04-29 11:40:32 PDT

Size: 17.29 KB

patch

obsolete

>Subversion Revision: 231153
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index c38034d7d393c6b8cc54f9c6ffd9882da0ea6a44..044d3254f7c7c43bd673e83e007ecd56f6a07894 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-04-29  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        Use WordLock instead of std::mutex for Threading
>+        https://bugs.webkit.org/show_bug.cgi?id=185121
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        ThreadGroup starts using WordLock.
>+
>+        * heap/MachineStackMarker.h:
>+        (JSC::MachineThreads::getLock):
>+
> 2018-04-29  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r231137.
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index d7130ca15f59522d849810bc0072f4040a72799b..ecfd623efdbe770cf654b4d6d0bdcee90afcd4de 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,49 @@
>+2018-04-29  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        Use WordLock instead of std::mutex for Threading
>+        https://bugs.webkit.org/show_bug.cgi?id=185121
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Before r231151, WordLock depends on ThreadSpecific. It means that our Threading implementation
>+        cannot use this lock since Threading primitives could touch these locks after ThreadSpecific
>+        for that WordLock is destroyed.
>+
>+        Now WordLock is changed not to use ThreadSpecific. So it does not depend on our Threading
>+        mechanism and our Threading can start using WordLock internally.
>+
>+        This patch changes WTF::Thread and WTF::ThreadGroup to use WordLock instead of std::mutex.
>+
>+        And add constexpr to explicitly describe that Lock, Condition, and WordLock constructors are constexpr.
>+
>+        * wtf/Condition.h:
>+        * wtf/Lock.h:
>+        * wtf/ThreadGroup.h:
>+        (WTF::ThreadGroup::getLock):
>+        * wtf/Threading.cpp:
>+        (WTF::Thread::didExit):
>+        (WTF::Thread::addToThreadGroup):
>+        (WTF::Thread::removeFromThreadGroup):
>+        * wtf/Threading.h:
>+        * wtf/ThreadingPthreads.cpp:
>+        (WTF::Thread::changePriority):
>+        (WTF::Thread::waitForCompletion):
>+        (WTF::Thread::detach):
>+        (WTF::Thread::signal):
>+        (WTF::Thread::establishPlatformSpecificHandle):
>+        * wtf/ThreadingWin.cpp:
>+        (WTF::Thread::changePriority):
>+        (WTF::Thread::waitForCompletion):
>+        (WTF::Thread::detach):
>+        (WTF::Thread::establishPlatformSpecificHandle):
>+        (WTF::Thread::initializeTLSKey):
>+        (WTF::Thread::currentDying):
>+        (WTF::Thread::get):
>+        (WTF::Thread::initializeTLS):
>+        (WTF::Thread::destructTLS):
>+        (WTF::threadMapMutex): Deleted.
>+        * wtf/WordLock.h:
>+
> 2018-04-29  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         [CMake] Require GCC 6
>diff --git a/Source/bmalloc/ChangeLog b/Source/bmalloc/ChangeLog
>index c3787cb871c9d19037d5bc9f34e0d1725300fcb3..cf31a65dfcc5ff26dd3bd9f4900c71f27dd6c26f 100644
>--- a/Source/bmalloc/ChangeLog
>+++ b/Source/bmalloc/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-04-29  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        Use WordLock instead of std::mutex for Threading
>+        https://bugs.webkit.org/show_bug.cgi?id=185121
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add constexpr to explicitly describe that bmalloc::Mutex constructor is constexpr.
>+
>+        * bmalloc/Mutex.h:
>+
> 2018-04-23  Ting-Wei Lan  <lantw44@gmail.com>
> 
>         Include stdio.h before using stderr
>diff --git a/Source/JavaScriptCore/heap/MachineStackMarker.h b/Source/JavaScriptCore/heap/MachineStackMarker.h
>index 1d794db1218fff8e227840c88868a78c7f556d32..5fe07c355cfae13fc0537b82c5d08947e0775890 100644
>--- a/Source/JavaScriptCore/heap/MachineStackMarker.h
>+++ b/Source/JavaScriptCore/heap/MachineStackMarker.h
>@@ -50,7 +50,7 @@ class MachineThreads {
>     // Only needs to be called by clients that can use the same heap from multiple threads.
>     void addCurrentThread() { m_threadGroup->addCurrentThread(); }
> 
>-    std::mutex& getLock() { return m_threadGroup->getLock(); }
>+    WordLock& getLock() { return m_threadGroup->getLock(); }
>     const ListHashSet<Ref<Thread>>& threads(const AbstractLocker& locker) const { return m_threadGroup->threads(locker); }
> 
> private:
>diff --git a/Source/WTF/wtf/Condition.h b/Source/WTF/wtf/Condition.h
>index 512f10f2ad53caeba8079d606f676cc9fc2c0edc..75c8ea5d91288a3b56d77ed3497aa2c39df104d8 100644
>--- a/Source/WTF/wtf/Condition.h
>+++ b/Source/WTF/wtf/Condition.h
>@@ -48,7 +48,7 @@ class Condition {
>     // are unlikely to be affected by the cost of conversions, it is better to use MonotonicTime.
>     using Time = ParkingLot::Time;
> 
>-    Condition() = default;
>+    constexpr Condition() = default;
> 
>     // Wait on a parking queue while releasing the given lock. It will unlock the lock just before
>     // parking, and relock it upon wakeup. Returns true if we woke up due to some call to
>diff --git a/Source/WTF/wtf/Lock.h b/Source/WTF/wtf/Lock.h
>index d3c72152c1c4d8187d004c2bb82a1027d24fa873..e39470e1e5d295a4521c7a0d12a17e54f1f78437 100644
>--- a/Source/WTF/wtf/Lock.h
>+++ b/Source/WTF/wtf/Lock.h
>@@ -52,7 +52,7 @@ class Lock {
>     WTF_MAKE_NONCOPYABLE(Lock);
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>-    Lock() = default;
>+    constexpr Lock() = default;
> 
>     void lock()
>     {
>diff --git a/Source/WTF/wtf/ThreadGroup.h b/Source/WTF/wtf/ThreadGroup.h
>index 88e419a111260774fd32cb180cb2778891a63aef..f53ce3196c2bdd6062a1b2adc66ef79b10cecf9a 100644
>--- a/Source/WTF/wtf/ThreadGroup.h
>+++ b/Source/WTF/wtf/ThreadGroup.h
>@@ -51,7 +51,7 @@ class ThreadGroup : public std::enable_shared_from_this<ThreadGroup> {
> 
>     const ListHashSet<Ref<Thread>>& threads(const AbstractLocker&) const { return m_threads; }
> 
>-    std::mutex& getLock() { return m_lock; }
>+    WordLock& getLock() { return m_lock; }
> 
>     WTF_EXPORT_PRIVATE ~ThreadGroup();
> 
>@@ -63,8 +63,8 @@ class ThreadGroup : public std::enable_shared_from_this<ThreadGroup> {
>         return shared_from_this();
>     }
> 
>-    // We use std::mutex since it can be used when deallocating TLS.
>-    std::mutex m_lock;
>+    // We use WordLock since it can be used when deallocating TLS.
>+    WordLock m_lock;
>     ListHashSet<Ref<Thread>> m_threads;
> };
> 
>diff --git a/Source/WTF/wtf/Threading.cpp b/Source/WTF/wtf/Threading.cpp
>index ee8ad94c4805d0a7346101587f8f6fac760ff1cc..d4ce4fc5264ee5c29bea6ad161c0398ee480663a 100644
>--- a/Source/WTF/wtf/Threading.cpp
>+++ b/Source/WTF/wtf/Threading.cpp
>@@ -180,7 +180,7 @@ void Thread::didExit()
>     if (shouldRemoveThreadFromThreadGroup()) {
>         Vector<std::shared_ptr<ThreadGroup>> threadGroups;
>         {
>-            std::lock_guard<std::mutex> locker(m_mutex);
>+            auto locker = holdLock(m_mutex);
>             for (auto& threadGroup : m_threadGroups) {
>                 // If ThreadGroup is just being destroyed,
>                 // we do not need to perform unregistering.
>@@ -190,8 +190,8 @@ void Thread::didExit()
>             m_isShuttingDown = true;
>         }
>         for (auto& threadGroup : threadGroups) {
>-            std::lock_guard<std::mutex> threadGroupLocker(threadGroup->getLock());
>-            std::lock_guard<std::mutex> locker(m_mutex);
>+            auto threadGroupLocker = holdLock(threadGroup->getLock());
>+            auto locker = holdLock(m_mutex);
>             threadGroup->m_threads.remove(*this);
>         }
>     }
>@@ -200,14 +200,14 @@ void Thread::didExit()
> 
>     // We would like to say "thread is exited" after unregistering threads from thread groups.
>     // So we need to separate m_isShuttingDown from m_didExit.
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     m_didExit = true;
> }
> 
> ThreadGroupAddResult Thread::addToThreadGroup(const AbstractLocker& threadGroupLocker, ThreadGroup& threadGroup)
> {
>     UNUSED_PARAM(threadGroupLocker);
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     if (m_isShuttingDown)
>         return ThreadGroupAddResult::NotAdded;
>     if (threadGroup.m_threads.add(*this).isNewEntry) {
>@@ -220,7 +220,7 @@ ThreadGroupAddResult Thread::addToThreadGroup(const AbstractLocker& threadGroupL
> void Thread::removeFromThreadGroup(const AbstractLocker& threadGroupLocker, ThreadGroup& threadGroup)
> {
>     UNUSED_PARAM(threadGroupLocker);
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     if (m_isShuttingDown)
>         return;
>     m_threadGroups.removeFirstMatching([&] (auto weakPtr) {
>diff --git a/Source/WTF/wtf/Threading.h b/Source/WTF/wtf/Threading.h
>index 97cbe5418b9cfba9b326ced471ac81fbe1ad4725..47e019fb95c88d454fc1b54f914d633f57b64417 100644
>--- a/Source/WTF/wtf/Threading.h
>+++ b/Source/WTF/wtf/Threading.h
>@@ -45,6 +45,7 @@
> #include <wtf/ThreadSafeRefCounted.h>
> #include <wtf/ThreadSpecific.h>
> #include <wtf/Vector.h>
>+#include <wtf/WordLock.h>
> 
> #if USE(PTHREADS) && !OS(DARWIN)
> #include <signal.h>
>@@ -273,8 +274,9 @@ class Thread : public ThreadSafeRefCounted<Thread> {
>     bool m_didExit { false };
>     bool m_isDestroyedOnce { false };
> 
>-    // WordLock & Lock rely on ThreadSpecific. But Thread object can be destroyed even after ThreadSpecific things are destroyed.
>-    std::mutex m_mutex;
>+    // Lock & ParkingLot rely on ThreadSpecific. But Thread object can be destroyed even after ThreadSpecific things are destroyed.
>+    // Use WordLock since WordLock does not depend on ThreadSpecific and this "Thread".
>+    WordLock m_mutex;
>     StackBounds m_stack { StackBounds::emptyBounds() };
>     Vector<std::weak_ptr<ThreadGroup>> m_threadGroups;
>     PlatformThreadHandle m_handle;
>diff --git a/Source/WTF/wtf/ThreadingPthreads.cpp b/Source/WTF/wtf/ThreadingPthreads.cpp
>index ddac7374a3c6f9525cee62063ff59eff1761787f..80b1212581c0692d5e5a462ad49363280683a4f5 100644
>--- a/Source/WTF/wtf/ThreadingPthreads.cpp
>+++ b/Source/WTF/wtf/ThreadingPthreads.cpp
>@@ -256,7 +256,7 @@ void Thread::initializeCurrentThreadInternal(const char* threadName)
> 
> void Thread::changePriority(int delta)
> {
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
> 
>     int policy;
>     struct sched_param param;
>@@ -273,7 +273,7 @@ int Thread::waitForCompletion()
> {
>     pthread_t handle;
>     {
>-        std::lock_guard<std::mutex> locker(m_mutex);
>+        auto locker = holdLock(m_mutex);
>         handle = m_handle;
>     }
> 
>@@ -284,7 +284,7 @@ int Thread::waitForCompletion()
>     else if (joinResult)
>         LOG_ERROR("Thread %p was unable to be joined.\n", this);
> 
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     ASSERT(joinableState() == Joinable);
> 
>     // If the thread has already exited, then do nothing. If the thread hasn't exited yet, then just signal that we've already joined on it.
>@@ -297,7 +297,7 @@ int Thread::waitForCompletion()
> 
> void Thread::detach()
> {
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     int detachResult = pthread_detach(m_handle);
>     if (detachResult)
>         LOG_ERROR("Thread %p was unable to be detached\n", this);
>@@ -319,7 +319,7 @@ Thread& Thread::initializeCurrentTLS()
> 
> bool Thread::signal(int signalNumber)
> {
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     if (hasExited())
>         return false;
>     int errNo = pthread_kill(m_handle, signalNumber);
>@@ -440,7 +440,7 @@ size_t Thread::getRegisters(PlatformRegisters& registers)
> 
> void Thread::establishPlatformSpecificHandle(pthread_t handle)
> {
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     m_handle = handle;
> #if OS(DARWIN)
>     m_platformThread = pthread_mach_thread_np(handle);
>diff --git a/Source/WTF/wtf/ThreadingWin.cpp b/Source/WTF/wtf/ThreadingWin.cpp
>index 6f3d41d6a0457fab0c06e8ed1b9d95c38c505c20..4e65a5e4277e4419e806652979695315998a56ee 100644
>--- a/Source/WTF/wtf/ThreadingWin.cpp
>+++ b/Source/WTF/wtf/ThreadingWin.cpp
>@@ -170,7 +170,7 @@ bool Thread::establishHandle(NewThreadContext* data)
> 
> void Thread::changePriority(int delta)
> {
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     SetThreadPriority(m_handle, THREAD_PRIORITY_NORMAL + delta);
> }
> 
>@@ -178,7 +178,7 @@ int Thread::waitForCompletion()
> {
>     HANDLE handle;
>     {
>-        std::lock_guard<std::mutex> locker(m_mutex);
>+        auto locker = holdLock(m_mutex);
>         handle = m_handle;
>     }
> 
>@@ -186,7 +186,7 @@ int Thread::waitForCompletion()
>     if (joinResult == WAIT_FAILED)
>         LOG_ERROR("Thread %p was found to be deadlocked trying to quit", this);
> 
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     ASSERT(joinableState() == Joinable);
> 
>     // The thread has already exited, do nothing.
>@@ -208,7 +208,7 @@ void Thread::detach()
>     // FlsCallback automatically. FlsCallback will call CloseHandle to clean up
>     // resource. So in this function, we just mark the thread as detached to
>     // avoid calling waitForCompletion for this thread.
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     if (!hasExited())
>         didBecomeDetached();
> }
>@@ -261,18 +261,14 @@ ThreadIdentifier Thread::currentID()
> 
> void Thread::establishPlatformSpecificHandle(HANDLE handle, ThreadIdentifier threadID)
> {
>-    std::lock_guard<std::mutex> locker(m_mutex);
>+    auto locker = holdLock(m_mutex);
>     m_handle = handle;
>     m_id = threadID;
> }
> 
> #define InvalidThread reinterpret_cast<Thread*>(static_cast<uintptr_t>(0xbbadbeef))
> 
>-static std::mutex& threadMapMutex()
>-{
>-    static NeverDestroyed<std::mutex> mutex;
>-    return mutex.get();
>-}
>+static WordLock threadMapMutex;
> 
> static HashMap<ThreadIdentifier, Thread*>& threadMap()
> {
>@@ -282,7 +278,6 @@ static HashMap<ThreadIdentifier, Thread*>& threadMap()
> 
> void Thread::initializeTLSKey()
> {
>-    threadMapMutex();
>     threadMap();
>     threadSpecificKeyCreate(&s_key, destructTLS);
> }
>@@ -291,14 +286,14 @@ Thread* Thread::currentDying()
> {
>     ASSERT(s_key != InvalidThreadSpecificKey);
>     // After FLS is destroyed, this map offers the value until the second thread exit callback is called.
>-    std::lock_guard<std::mutex> locker(threadMapMutex());
>+    auto locker = holdLock(threadMapMutex);
>     return threadMap().get(currentID());
> }
> 
> // FIXME: Remove this workaround code once <rdar://problem/31793213> is fixed.
> RefPtr<Thread> Thread::get(ThreadIdentifier id)
> {
>-    std::lock_guard<std::mutex> locker(threadMapMutex());
>+    auto locker = holdLock(threadMapMutex);
>     Thread* thread = threadMap().get(id);
>     if (thread)
>         return thread;
>@@ -314,7 +309,7 @@ Thread& Thread::initializeTLS(Ref<Thread>&& thread)
>     auto& threadInTLS = thread.leakRef();
>     threadSpecificSet(s_key, &threadInTLS);
>     {
>-        std::lock_guard<std::mutex> locker(threadMapMutex());
>+        auto locker = holdLock(threadMapMutex);
>         threadMap().add(id, &threadInTLS);
>     }
>     return threadInTLS;
>@@ -348,7 +343,7 @@ void Thread::destructTLS(void* data)
> 
>     if (thread->m_isDestroyedOnce) {
>         {
>-            std::lock_guard<std::mutex> locker(threadMapMutex());
>+            auto locker = holdLock(threadMapMutex);
>             ASSERT(threadMap().contains(thread->id()));
>             threadMap().remove(thread->id());
>         }
>diff --git a/Source/WTF/wtf/WordLock.h b/Source/WTF/wtf/WordLock.h
>index bf1354a6b86ce72d6ac7be1430a9359cf2343e24..8a90c1e81aa9f6463242e56dc20827af1ef45638 100644
>--- a/Source/WTF/wtf/WordLock.h
>+++ b/Source/WTF/wtf/WordLock.h
>@@ -50,7 +50,7 @@ class WordLock {
>     WTF_MAKE_NONCOPYABLE(WordLock);
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>-    WordLock() = default;
>+    constexpr WordLock() = default;
> 
>     void lock()
>     {
>diff --git a/Source/bmalloc/bmalloc/Mutex.h b/Source/bmalloc/bmalloc/Mutex.h
>index a8c5ee6b02a672a3e7b448d7ef2a9a4a4ff1fc31..e18de8408fb9d66020966bc68cc744052b68b738 100644
>--- a/Source/bmalloc/bmalloc/Mutex.h
>+++ b/Source/bmalloc/bmalloc/Mutex.h
>@@ -37,7 +37,7 @@ namespace bmalloc {
> 
> class Mutex {
> public:
>-    Mutex() = default;
>+    constexpr Mutex() = default;
> 
>     void lock();
>     bool try_lock();
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 5f79440757d43bbc2a53367644569ddff278741a..3353ccecec9b9a4a1d423da8540516764f305d93 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-04-29  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        Use WordLock instead of std::mutex for Threading
>+        https://bugs.webkit.org/show_bug.cgi?id=185121
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/Tests/WTF/Signals.cpp:
>+        (TEST):
>+
> 2018-04-28  Sihui Liu  <sihui_liu@apple.com>
> 
>         [Cocoa] Set HTTPOnly flag when converting Cookie to NSHTTPCookie
>diff --git a/Tools/TestWebKitAPI/Tests/WTF/Signals.cpp b/Tools/TestWebKitAPI/Tests/WTF/Signals.cpp
>index 4b9e834029db70f0592935e741a6c2c63cdd2b25..680c9f595014d6bbf867373f23eeb01e2fe92010 100644
>--- a/Tools/TestWebKitAPI/Tests/WTF/Signals.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WTF/Signals.cpp
>@@ -54,7 +54,7 @@ TEST(Signals, SignalsWorkOnExit)
> 
>     bool signalFired;
>     {
>-        std::unique_lock<std::mutex> locker(static_cast<ReflectedThread&>(receiverThread.get()).m_mutex);
>+        std::unique_lock<WordLock> locker(static_cast<ReflectedThread&>(receiverThread.get()).m_mutex);
>         receiverShouldKeepRunning.store(false);
>         EXPECT_FALSE(static_cast<ReflectedThread&>(receiverThread.get()).hasExited());
>         sleep(1_s);

Actions: View | Formatted Diff | Diff

Attachments on bug 185121: 339094 | 339097