Changeset 209938 in webkit

Timestamp:

Dec 16, 2016 2:26:09 PM (7 years ago)

Author:

msaboff@apple.com

Message:

REGRESSION: HipChat and Mail sometimes hang beneath JSC::Heap::lastChanceToFinalize()
​https://bugs.webkit.org/show_bug.cgi?id=165962

Reviewed by Filip Pizlo.

There is an inherent race in Condition::waitFor() where the timeout can happen just before
a notify from another thread.

Fixed this by adding a condition variable and flag to each AutomaticThread. The flag
is used to signify to a notifying thread that the thread is waiting. That flag is set
in the waiting thread before calling waitFor() and cleared by another thread when it
notifies the thread. The access to that flag happens when the lock is held.
Now the waiting thread checks if the flag after a timeout to see that it in fact should
proceed like a normal notification.

The added condition variable allows us to target a specific thread. We used to keep a list
of waiting threads, now we keep a list of all threads. To notify one thread, we look for
a waiting thread and notify it directly. If we can't find a waiting thread, we start a
sleeping thread.

We notify all threads by waking all waiting threads and starting all sleeping threads.

• wtf/AutomaticThread.cpp:

(WTF::AutomaticThreadCondition::notifyOne):
(WTF::AutomaticThreadCondition::notifyAll):
(WTF::AutomaticThread::isWaiting):
(WTF::AutomaticThread::notify):
(WTF::AutomaticThread::start):

• wtf/AutomaticThread.h:

Location:

trunk/Source/WTF

Files:

• ChangeLog (modified) (1 diff)
• wtf/AutomaticThread.cpp (modified) (5 diffs)
• wtf/AutomaticThread.h (modified) (2 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2016-12-16  Michael Saboff  <msaboff@apple.com>
+
+        REGRESSION: HipChat and Mail sometimes hang beneath JSC::Heap::lastChanceToFinalize()
+        https://bugs.webkit.org/show_bug.cgi?id=165962
+
+        Reviewed by Filip Pizlo.
+
+        There is an inherent race in Condition::waitFor() where the timeout can happen just before
+        a notify from another thread.
+
+        Fixed this by adding a condition variable and flag to each AutomaticThread.  The flag
+        is used to signify to a notifying thread that the thread is waiting.  That flag is set
+        in the waiting thread before calling waitFor() and cleared by another thread when it
+        notifies the thread.  The access to that flag happens when the lock is held.
+        Now the waiting thread checks if the flag after a timeout to see that it in fact should
+        proceed like a normal notification.
+
+        The added condition variable allows us to target a specific thread.  We used to keep a list
+        of waiting threads, now we keep a list of all threads.  To notify one thread, we look for
+        a waiting thread and notify it directly.  If we can't find a waiting thread, we start a
+        sleeping thread.
+
+        We notify all threads by waking all waiting threads and starting all sleeping threads.
+
+        * wtf/AutomaticThread.cpp:
+        (WTF::AutomaticThreadCondition::notifyOne):
+        (WTF::AutomaticThreadCondition::notifyAll):
+        (WTF::AutomaticThread::isWaiting):
+        (WTF::AutomaticThread::notify):
+        (WTF::AutomaticThread::start):
+        * wtf/AutomaticThread.h:
+
 2016-12-15  Myles C. Maxfield  <mmaxfield@apple.com>
 

trunk/Source/WTF/wtf/AutomaticThread.cpp

@@ -48 +48 @@
 void AutomaticThreadCondition::notifyOne(const LockHolder& locker)
 {
-    if (m_condition.notifyOne())
-        return;
-    
-    if (m_threads.isEmpty())
-        return;
-    
-    m_threads.takeLast()->start(locker);
+    for (AutomaticThread* thread : m_threads) {
+        if (thread->isWaiting(locker)) {
+            thread->notify(locker);
+            return;
+        }
+    }
+
+    for (AutomaticThread* thread : m_threads) {
+        if (!thread->hasUnderlyingThread(locker)) {
+            thread->start(locker);
+            return;
+        }
+    }
+
+    m_condition.notifyOne();
 }
 
@@ -60 +68 @@
 {
     m_condition.notifyAll();
-    
-    for (AutomaticThread* thread : m_threads)
-        thread->start(locker);
-    m_threads.clear();
+
+    for (AutomaticThread* thread : m_threads) {
+        if (thread->isWaiting(locker))
+            thread->notify(locker);
+        else if (!thread->hasUnderlyingThread(locker))
+            thread->start(locker);
+    }
 }
 
@@ -116 +127 @@
     m_isRunning = false;
     return true;
+}
+
+bool AutomaticThread::isWaiting(const LockHolder& locker)
+{
+    return hasUnderlyingThread(locker) && m_isWaiting;
+}
+
+bool AutomaticThread::notify(const LockHolder& locker)
+{
+    ASSERT_UNUSED(locker, hasUnderlyingThread(locker));
+    m_isWaiting = false;
+    return m_waitCondition.notifyOne();
 }
 
@@ -162 +185 @@
             if (!ASSERT_DISABLED) {
                 LockHolder locker(*m_lock);
-                ASSERT(!m_condition->contains(locker, this));
+                ASSERT(m_condition->contains(locker, this));
             }
             
@@ -181 +204 @@
                         RELEASE_ASSERT(result == PollResult::Wait);
                         // Shut the thread down after one second.
+                        m_isWaiting = true;
                         bool awokenByNotify =
-                            m_condition->m_condition.waitFor(*m_lock, 1_s);
-                        if (!awokenByNotify) {
+                            m_waitCondition.waitFor(*m_lock, 1_s);
+                        if (verbose && !awokenByNotify && !m_isWaiting)
+                            dataLog(RawPointer(this), ": waitFor timed out, but notified via m_isWaiting flag!\n");
+                        if (m_isWaiting) {
+                            m_isWaiting = false;
                             if (verbose)
                                 dataLog(RawPointer(this), ": Going to sleep!\n");
-                            m_condition->add(locker, this);
                             return;
                         }

trunk/Source/WTF/wtf/AutomaticThread.h

@@ -121 +121 @@
     // mechanism (set some flag and then notify the condition).
     bool tryStop(const LockHolder&);
-    
+
+    bool isWaiting(const LockHolder&);
+
+    bool notify(const LockHolder&);
+
     void join();
     
@@ -178 +182 @@
     RefPtr<AutomaticThreadCondition> m_condition;
     bool m_isRunning { true };
+    bool m_isWaiting { false };
     bool m_hasUnderlyingThread { false };
+    Condition m_waitCondition;
     Condition m_isRunningCondition;
 };