Attachment 339092 Details for Bug 185119 – Patch

[patch] Patch

bug-185119-20180429084146.patch (text/plain), 4.40 KB, created by Geoffrey Garen on 2018-04-29 08:41:46 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Geoffrey Garen

Created: 2018-04-29 08:41:46 PDT

Size: 4.40 KB

patch

obsolete

>Index: Source/WTF/ChangeLog
>===================================================================
>--- Source/WTF/ChangeLog	(revision 231149)
>+++ Source/WTF/ChangeLog	(working copy)
>@@ -1,3 +1,21 @@
>+2018-04-29  Geoffrey Garen  <ggaren@apple.com>
>+
>+        WordLock doesn't need per-thread data
>+        https://bugs.webkit.org/show_bug.cgi?id=185119
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The stack is per-thread data, so we can stack-allocate our ThreadData.
>+
>+        This eliminates malloc() and high-level WTF threading primitives from
>+        WordLock, making WordLock more portable to non-WTF code, including
>+        bmalloc.
>+
>+        (NOTE: This patch makes the bug fixed in r231148 100% reproducible.)
>+
>+        * wtf/WordLock.cpp:
>+        (WTF::WordLock::lockSlow): Allocate ThreadData on the stack.
>+
> 2018-04-28  Geoffrey Garen  <ggaren@apple.com>
> 
>         Fixed a very unlikely race condition in WTF::WordLock
>Index: Source/WTF/wtf/WordLock.cpp
>===================================================================
>--- Source/WTF/wtf/WordLock.cpp	(revision 231149)
>+++ Source/WTF/wtf/WordLock.cpp	(working copy)
>@@ -26,9 +26,7 @@
> #include "config.h"
> #include "WordLock.h"
> 
>-#include "ThreadSpecific.h"
> #include "Threading.h"
>-#include "ThreadingPrimitives.h"
> #include <condition_variable>
> #include <mutex>
> #include <thread>
>@@ -60,20 +58,6 @@ struct ThreadData {
>     ThreadData* queueTail { nullptr };
> };
> 
>-ThreadSpecific<ThreadData, CanBeGCThread::True>* threadData;
>-
>-ThreadData* myThreadData()
>-{
>-    static std::once_flag initializeOnce;
>-    std::call_once(
>-        initializeOnce,
>-        [] {
>-            threadData = new ThreadSpecific<ThreadData, CanBeGCThread::True>();
>-        });
>-
>-    return *threadData;
>-}
>-
> } // anonymous namespace
> 
> NEVER_INLINE void WordLock::lockSlow()
>@@ -106,12 +90,8 @@ NEVER_INLINE void WordLock::lockSlow()
> 
>         // Need to put ourselves on the queue. Create the queue if one does not exist. This requries
>         // owning the queue for a little bit. The lock that controls the queue is itself a spinlock.
>-        // But before we acquire the queue spinlock, we make sure that we have a ThreadData for this
>-        // thread.
>-        ThreadData* me = myThreadData();
>-        ASSERT(!me->shouldPark);
>-        ASSERT(!me->nextInQueue);
>-        ASSERT(!me->queueTail);
>+
>+        ThreadData me;
> 
>         // Reload the current word value, since some time may have passed.
>         currentWordValue = m_word.load();
>@@ -125,15 +105,15 @@ NEVER_INLINE void WordLock::lockSlow()
>             continue;
>         }
>         
>-        me->shouldPark = true;
>+        me.shouldPark = true;
> 
>         // We own the queue. Nobody can enqueue or dequeue until we're done. Also, it's not possible
>         // to release the WordLock while we hold the queue lock.
>         ThreadData* queueHead = bitwise_cast<ThreadData*>(currentWordValue & ~queueHeadMask);
>         if (queueHead) {
>             // Put this thread at the end of the queue.
>-            queueHead->queueTail->nextInQueue = me;
>-            queueHead->queueTail = me;
>+            queueHead->queueTail->nextInQueue = &me;
>+            queueHead->queueTail = &me;
> 
>             // Release the queue lock.
>             currentWordValue = m_word.load();
>@@ -143,8 +123,8 @@ NEVER_INLINE void WordLock::lockSlow()
>             m_word.store(currentWordValue & ~isQueueLockedBit);
>         } else {
>             // Make this thread be the queue-head.
>-            queueHead = me;
>-            me->queueTail = me;
>+            queueHead = &me;
>+            me.queueTail = &me;
> 
>             // Release the queue lock and install ourselves as the head. No need for a CAS loop, since
>             // we own the queue lock.
>@@ -164,14 +144,14 @@ NEVER_INLINE void WordLock::lockSlow()
>         // releasing thread holds me's parkingLock.
> 
>         {
>-            std::unique_lock<std::mutex> locker(me->parkingLock);
>-            while (me->shouldPark)
>-                me->parkingCondition.wait(locker);
>+            std::unique_lock<std::mutex> locker(me.parkingLock);
>+            while (me.shouldPark)
>+                me.parkingCondition.wait(locker);
>         }
> 
>-        ASSERT(!me->shouldPark);
>-        ASSERT(!me->nextInQueue);
>-        ASSERT(!me->queueTail);
>+        ASSERT(!me.shouldPark);
>+        ASSERT(!me.nextInQueue);
>+        ASSERT(!me.queueTail);
>         
>         // Now we can loop around and try to acquire the lock again.
>     }

Actions: View | Formatted Diff | Diff

Attachments on bug 185119: 339092