<!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>[197381] trunk/Source/JavaScriptCore</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/197381">197381</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2016-02-29 19:18:59 -0800 (Mon, 29 Feb 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>regress/script-tests/double-pollution-putbyoffset.js.ftl-eager timed out because of a lock ordering deadlock involving InferredType and CodeBlock
https://bugs.webkit.org/show_bug.cgi?id=154841
Reviewed by Benjamin Poulain.
Here's the deadlock:
Main thread:
1) Change an InferredType. This acquires InferredType::m_lock.
2) Fire watchpoint set. This triggers CodeBlock invalidation, which acquires
CodeBlock::m_lock.
DFG thread:
1) Iterate over the information in a CodeBlock. This acquires CodeBlock::m_lock.
2) Ask an InferredType for its descriptor(). This acquires InferredType::m_lock.
I think that the DFG thread's ordering should be legal, because the best logic for lock
hierarchies is that locks that protect the largest set of stuff should be acquired first.
This means that the main thread shouldn't be holding the InferredType::m_lock when firing
watchpoint sets. That's what this patch ensures.
At the time of writing, this test was deadlocking for me on trunk 100% of the time. With
this change I cannot get it to deadlock.
* runtime/InferredType.cpp:
(JSC::InferredType::willStoreValueSlow):
(JSC::InferredType::makeTopSlow):
(JSC::InferredType::set):
(JSC::InferredType::removeStructure):
(JSC::InferredType::InferredStructureWatchpoint::fireInternal):
* runtime/InferredType.h:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeInferredTypecpp">trunk/Source/JavaScriptCore/runtime/InferredType.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeInferredTypeh">trunk/Source/JavaScriptCore/runtime/InferredType.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (197380 => 197381)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-03-01 02:30:46 UTC (rev 197380)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-03-01 03:18:59 UTC (rev 197381)
</span><span class="lines">@@ -1,3 +1,38 @@
</span><ins>+2016-02-29 Filip Pizlo <fpizlo@apple.com>
+
+ regress/script-tests/double-pollution-putbyoffset.js.ftl-eager timed out because of a lock ordering deadlock involving InferredType and CodeBlock
+ https://bugs.webkit.org/show_bug.cgi?id=154841
+
+ Reviewed by Benjamin Poulain.
+
+ Here's the deadlock:
+
+ Main thread:
+ 1) Change an InferredType. This acquires InferredType::m_lock.
+ 2) Fire watchpoint set. This triggers CodeBlock invalidation, which acquires
+ CodeBlock::m_lock.
+
+ DFG thread:
+ 1) Iterate over the information in a CodeBlock. This acquires CodeBlock::m_lock.
+ 2) Ask an InferredType for its descriptor(). This acquires InferredType::m_lock.
+
+ I think that the DFG thread's ordering should be legal, because the best logic for lock
+ hierarchies is that locks that protect the largest set of stuff should be acquired first.
+
+ This means that the main thread shouldn't be holding the InferredType::m_lock when firing
+ watchpoint sets. That's what this patch ensures.
+
+ At the time of writing, this test was deadlocking for me on trunk 100% of the time. With
+ this change I cannot get it to deadlock.
+
+ * runtime/InferredType.cpp:
+ (JSC::InferredType::willStoreValueSlow):
+ (JSC::InferredType::makeTopSlow):
+ (JSC::InferredType::set):
+ (JSC::InferredType::removeStructure):
+ (JSC::InferredType::InferredStructureWatchpoint::fireInternal):
+ * runtime/InferredType.h:
+
</ins><span class="cx"> 2016-02-29 Yusuke Suzuki <utatane.tea@gmail.com>
</span><span class="cx">
</span><span class="cx"> [DFG][FTL][B3] Support floor and ceil
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeInferredTypecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/InferredType.cpp (197380 => 197381)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/InferredType.cpp 2016-03-01 02:30:46 UTC (rev 197380)
+++ trunk/Source/JavaScriptCore/runtime/InferredType.cpp 2016-03-01 03:18:59 UTC (rev 197381)
</span><span class="lines">@@ -400,28 +400,44 @@
</span><span class="cx">
</span><span class="cx"> bool InferredType::willStoreValueSlow(VM& vm, PropertyName propertyName, JSValue value)
</span><span class="cx"> {
</span><del>- ConcurrentJITLocker locker(m_lock);
- Descriptor myType = descriptor(locker);
- Descriptor otherType = Descriptor::forValue(value);
</del><ins>+ Descriptor oldType;
+ Descriptor myType;
+ bool result;
+ {
+ ConcurrentJITLocker locker(m_lock);
+ oldType = descriptor(locker);
+ myType = Descriptor::forValue(value);
</ins><span class="cx">
</span><del>- myType.merge(otherType);
</del><ins>+ myType.merge(oldType);
+
+ ASSERT(oldType != myType); // The type must have changed if we're on the slow path.
</ins><span class="cx">
</span><del>- ASSERT(descriptor(locker) != myType); // The type must have changed if we're on the slow path.
-
- set(locker, vm, propertyName, value, myType);
-
- return kind(locker) != Top;
</del><ins>+ bool setResult = set(locker, vm, myType);
+ result = kind(locker) != Top;
+ if (!setResult)
+ return result;
+ }
+
+ InferredTypeFireDetail detail(this, propertyName.uid(), oldType, myType, value);
+ m_watchpointSet.fireAll(detail);
+ return result;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void InferredType::makeTopSlow(VM& vm, PropertyName propertyName)
</span><span class="cx"> {
</span><del>- ConcurrentJITLocker locker(m_lock);
- set(locker, vm, propertyName, JSValue(), Top);
</del><ins>+ Descriptor oldType;
+ {
+ ConcurrentJITLocker locker(m_lock);
+ oldType = descriptor(locker);
+ if (!set(locker, vm, Top))
+ return;
+ }
+
+ InferredTypeFireDetail detail(this, propertyName.uid(), oldType, Top, JSValue());
+ m_watchpointSet.fireAll(detail);
</ins><span class="cx"> }
</span><span class="cx">
</span><del>-void InferredType::set(
- const ConcurrentJITLocker& locker, VM& vm, PropertyName propertyName, JSValue offendingValue,
- Descriptor newDescriptor)
</del><ins>+bool InferredType::set(const ConcurrentJITLocker& locker, VM& vm, Descriptor newDescriptor)
</ins><span class="cx"> {
</span><span class="cx"> // We will trigger write barriers while holding our lock. Currently, write barriers don't GC, but that
</span><span class="cx"> // could change. If it does, we don't want to deadlock. Note that we could have used
</span><span class="lines">@@ -431,8 +447,10 @@
</span><span class="cx">
</span><span class="cx"> // Be defensive: if we're not really changing the type, then we don't have to do anything.
</span><span class="cx"> if (descriptor(locker) == newDescriptor)
</span><del>- return;
</del><ins>+ return false;
</ins><span class="cx">
</span><ins>+ bool shouldFireWatchpointSet = false;
+
</ins><span class="cx"> // The new descriptor must be more general than the previous one.
</span><span class="cx"> ASSERT(newDescriptor.subsumes(descriptor(locker)));
</span><span class="cx">
</span><span class="lines">@@ -446,15 +464,12 @@
</span><span class="cx"> // We cannot have been invalidated, since if we were, then we'd already be at Top.
</span><span class="cx"> ASSERT(m_watchpointSet.state() != IsInvalidated);
</span><span class="cx">
</span><del>- InferredTypeFireDetail detail(
- this, propertyName.uid(), descriptor(locker), newDescriptor, offendingValue);
-
</del><span class="cx"> // We're about to do expensive things because some compiler thread decided to watch this type and
</span><span class="cx"> // then the type changed. Assume that this property is crazy, and don't ever do any more things for
</span><span class="cx"> // it.
</span><span class="cx"> newDescriptor = Top;
</span><span class="cx">
</span><del>- m_watchpointSet.fireAll(detail);
</del><ins>+ shouldFireWatchpointSet = true;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // Remove the old InferredStructure object if we no longer need it.
</span><span class="lines">@@ -477,6 +492,8 @@
</span><span class="cx">
</span><span class="cx"> // Assert that we did things.
</span><span class="cx"> ASSERT(descriptor(locker) == newDescriptor);
</span><ins>+
+ return shouldFireWatchpointSet;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void InferredType::removeStructure()
</span><span class="lines">@@ -486,12 +503,20 @@
</span><span class="cx">
</span><span class="cx"> VM& vm = *Heap::heap(this)->vm();
</span><span class="cx">
</span><del>- ConcurrentJITLocker locker(m_lock);
-
- Descriptor newDescriptor = descriptor(locker);
- newDescriptor.removeStructure();
-
- set(locker, vm, PropertyName(nullptr), JSValue(), newDescriptor);
</del><ins>+ Descriptor oldDescriptor;
+ Descriptor newDescriptor;
+ {
+ ConcurrentJITLocker locker(m_lock);
+ oldDescriptor = descriptor(locker);
+ newDescriptor = oldDescriptor;
+ newDescriptor.removeStructure();
+
+ if (!set(locker, vm, newDescriptor))
+ return;
+ }
+
+ InferredTypeFireDetail detail(this, nullptr, oldDescriptor, newDescriptor, JSValue());
+ m_watchpointSet.fireAll(detail);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void InferredType::InferredStructureWatchpoint::fireInternal(const FireDetail&)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeInferredTypeh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/InferredType.h (197380 => 197381)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/InferredType.h 2016-03-01 02:30:46 UTC (rev 197380)
+++ trunk/Source/JavaScriptCore/runtime/InferredType.h 2016-03-01 03:18:59 UTC (rev 197381)
</span><span class="lines">@@ -229,7 +229,11 @@
</span><span class="cx">
</span><span class="cx"> bool willStoreValueSlow(VM&, PropertyName, JSValue);
</span><span class="cx"> void makeTopSlow(VM&, PropertyName);
</span><del>- void set(const ConcurrentJITLocker&, VM&, PropertyName, JSValue, Descriptor);
</del><ins>+
+ // Helper for willStoreValueSlow() and makeTopSlow(). This returns true if we should fire the
+ // watchpoint set.
+ bool set(const ConcurrentJITLocker&, VM&, Descriptor);
+
</ins><span class="cx"> void removeStructure();
</span><span class="cx">
</span><span class="cx"> mutable ConcurrentJITLock m_lock;
</span></span></pre>
</div>
</div>
</body>
</html>