<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[209938] trunk/Source/WTF</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/209938">209938</a></dd>
<dt>Author</dt> <dd>msaboff@apple.com</dd>
<dt>Date</dt> <dd>2016-12-16 14:26:09 -0800 (Fri, 16 Dec 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>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:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWTFChangeLog">trunk/Source/WTF/ChangeLog</a></li>
<li><a href="#trunkSourceWTFwtfAutomaticThreadcpp">trunk/Source/WTF/wtf/AutomaticThread.cpp</a></li>
<li><a href="#trunkSourceWTFwtfAutomaticThreadh">trunk/Source/WTF/wtf/AutomaticThread.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWTFChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/ChangeLog (209937 => 209938)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/ChangeLog 2016-12-16 22:05:54 UTC (rev 209937)
+++ trunk/Source/WTF/ChangeLog 2016-12-16 22:26:09 UTC (rev 209938)
</span><span class="lines">@@ -1,3 +1,35 @@
</span><ins>+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:
+
</ins><span class="cx"> 2016-12-15 Myles C. Maxfield <mmaxfield@apple.com>
</span><span class="cx">
</span><span class="cx"> Unreviewed build fix after r209910
</span></span></pre></div>
<a id="trunkSourceWTFwtfAutomaticThreadcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/AutomaticThread.cpp (209937 => 209938)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/AutomaticThread.cpp 2016-12-16 22:05:54 UTC (rev 209937)
+++ trunk/Source/WTF/wtf/AutomaticThread.cpp 2016-12-16 22:26:09 UTC (rev 209938)
</span><span class="lines">@@ -47,22 +47,33 @@
</span><span class="cx">
</span><span class="cx"> void AutomaticThreadCondition::notifyOne(const LockHolder& locker)
</span><span class="cx"> {
</span><del>- if (m_condition.notifyOne())
- return;
-
- if (m_threads.isEmpty())
- return;
-
- m_threads.takeLast()->start(locker);
</del><ins>+ 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();
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void AutomaticThreadCondition::notifyAll(const LockHolder& locker)
</span><span class="cx"> {
</span><span class="cx"> m_condition.notifyAll();
</span><del>-
- for (AutomaticThread* thread : m_threads)
- thread->start(locker);
- m_threads.clear();
</del><ins>+
+ for (AutomaticThread* thread : m_threads) {
+ if (thread->isWaiting(locker))
+ thread->notify(locker);
+ else if (!thread->hasUnderlyingThread(locker))
+ thread->start(locker);
+ }
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void AutomaticThreadCondition::wait(Lock& lock)
</span><span class="lines">@@ -117,6 +128,18 @@
</span><span class="cx"> return true;
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+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();
+}
+
</ins><span class="cx"> void AutomaticThread::join()
</span><span class="cx"> {
</span><span class="cx"> LockHolder locker(*m_lock);
</span><span class="lines">@@ -161,7 +184,7 @@
</span><span class="cx">
</span><span class="cx"> if (!ASSERT_DISABLED) {
</span><span class="cx"> LockHolder locker(*m_lock);
</span><del>- ASSERT(!m_condition->contains(locker, this));
</del><ins>+ ASSERT(m_condition->contains(locker, this));
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> auto stop = [&] (const LockHolder&) {
</span><span class="lines">@@ -180,12 +203,15 @@
</span><span class="cx"> return stop(locker);
</span><span class="cx"> RELEASE_ASSERT(result == PollResult::Wait);
</span><span class="cx"> // Shut the thread down after one second.
</span><ins>+ m_isWaiting = true;
</ins><span class="cx"> bool awokenByNotify =
</span><del>- m_condition->m_condition.waitFor(*m_lock, 1_s);
- if (!awokenByNotify) {
</del><ins>+ 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;
</ins><span class="cx"> if (verbose)
</span><span class="cx"> dataLog(RawPointer(this), ": Going to sleep!\n");
</span><del>- m_condition->add(locker, this);
</del><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceWTFwtfAutomaticThreadh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/AutomaticThread.h (209937 => 209938)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/AutomaticThread.h 2016-12-16 22:05:54 UTC (rev 209937)
+++ trunk/Source/WTF/wtf/AutomaticThread.h 2016-12-16 22:26:09 UTC (rev 209938)
</span><span class="lines">@@ -120,7 +120,11 @@
</span><span class="cx"> // thread is to first try this, and if that doesn't work, to tell the thread using your own
</span><span class="cx"> // mechanism (set some flag and then notify the condition).
</span><span class="cx"> bool tryStop(const LockHolder&);
</span><del>-
</del><ins>+
+ bool isWaiting(const LockHolder&);
+
+ bool notify(const LockHolder&);
+
</ins><span class="cx"> void join();
</span><span class="cx">
</span><span class="cx"> protected:
</span><span class="lines">@@ -177,7 +181,9 @@
</span><span class="cx"> Box<Lock> m_lock;
</span><span class="cx"> RefPtr<AutomaticThreadCondition> m_condition;
</span><span class="cx"> bool m_isRunning { true };
</span><ins>+ bool m_isWaiting { false };
</ins><span class="cx"> bool m_hasUnderlyingThread { false };
</span><ins>+ Condition m_waitCondition;
</ins><span class="cx"> Condition m_isRunningCondition;
</span><span class="cx"> };
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>