<!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>[193470] 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/193470">193470</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2015-12-04 14:25:26 -0800 (Fri, 04 Dec 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>Having a bad time has a really awful time when it runs at the same time as the JIT
https://bugs.webkit.org/show_bug.cgi?id=151882
rdar://problem/23547038
Reviewed by Geoffrey Garen.
The DFG's use of watchpoints for havingABadTime goes back a long time. We introduced this feature
when we first introduced watchpoints. That left it open to a lot of bitrot. On top of that, this
code doesn't get tested much because having a bad time is not something that is really supposed to
happen.
Well, now I've got reports that it does happen - or at least, we know that it is because of
crashes in an assertion that could only be triggered if a bad time was had. In the meantime, we
added two new features without adequately testing havingABadTime: concurrent JIT and FTL.
Concurrency means that we have to worry about the havingABadTime watchpoint triggering during
compilation. FTL means that we have new code and new optimizations that needs to deal with this
feature correctly.
The bug can arise via race condition or just goofy profiling. As in the newly added test, we could
first profile an allocation thinking that it will allocate sane arrays. Then we might have a bad
time, and then compile that function with the FTL. The ByteCodeParser will represent the
allocation with a NewArray node that has a sane indexingType(). But when we go to lower the Node,
we observe that the Structure* that the JSGlobalObject tells us to use has a different indexing
type. This is a feature of havingABadTime that the DFG knew about, but the FTL didn't. The FTL
didn't know about it because we didn't have adequate tests, and this code rarely gets triggered in
the wild. So, the FTL had a silly assertion that the indexing types match. They absolutely don't
have to match.
There is another bug, a race condition, that remains even if we remove the bad assertion. We set
the havingABadTime watchpoint late in compilation, and we do it based on whether the watchpoint is
still OK. This means that we could parse a function before we have a bad time and then do
optimizations (for example in AbstractInterpreter) like proving that the structure set associated
with the value returned by the NewArray is the one with a sane indexing type. Then, after those
optimizations have already been done, we will go to set the watchpoint. But just as we are doing
this, we could haveABadTime on the main thread. Currently this sort of almost works because
having a bad time requires doing a GC, and we can't GC until the watchpoint collection phase. But
that feels too fragile. So, this phase moves the setting of the watchpoint to the FixupPhase. This
is consistent with our long-term goal of removing the WatchpointCollectionPhase. Moving this to
FixupPhase means that we set the watchpoint before doing any optimizations. So, if having a bad
time happens before the FixupPhase then all optimizations will agree that we're having a bad time
and so everything is fine; if we have a bad time after FixupPhase then we will cancel the
compilation anyway.
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleConstantInternalFunction):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::watchHavingABadTime):
(JSC::DFG::FixupPhase::createToString):
* dfg/DFGNode.h:
(JSC::DFG::Node::hasIndexingType):
(JSC::DFG::Node::indexingType):
* dfg/DFGWatchpointCollectionPhase.cpp:
(JSC::DFG::WatchpointCollectionPhase::handle):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compileNewArray):
(JSC::FTL::DFG::LowerDFGToLLVM::compileNewArrayBuffer):
* tests/stress/ftl-has-a-bad-time.js: Added.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGByteCodeParsercpp">trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGFixupPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGNodeh">trunk/Source/JavaScriptCore/dfg/DFGNode.h</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGWatchpointCollectionPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGWatchpointCollectionPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreftlFTLLowerDFGToLLVMcpp">trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (193469 => 193470)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-12-04 22:15:23 UTC (rev 193469)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-12-04 22:25:26 UTC (rev 193470)
</span><span class="lines">@@ -1,3 +1,64 @@
</span><ins>+2015-12-04 Filip Pizlo <fpizlo@apple.com>
+
+ Having a bad time has a really awful time when it runs at the same time as the JIT
+ https://bugs.webkit.org/show_bug.cgi?id=151882
+ rdar://problem/23547038
+
+ Reviewed by Geoffrey Garen.
+
+ The DFG's use of watchpoints for havingABadTime goes back a long time. We introduced this feature
+ when we first introduced watchpoints. That left it open to a lot of bitrot. On top of that, this
+ code doesn't get tested much because having a bad time is not something that is really supposed to
+ happen.
+
+ Well, now I've got reports that it does happen - or at least, we know that it is because of
+ crashes in an assertion that could only be triggered if a bad time was had. In the meantime, we
+ added two new features without adequately testing havingABadTime: concurrent JIT and FTL.
+ Concurrency means that we have to worry about the havingABadTime watchpoint triggering during
+ compilation. FTL means that we have new code and new optimizations that needs to deal with this
+ feature correctly.
+
+ The bug can arise via race condition or just goofy profiling. As in the newly added test, we could
+ first profile an allocation thinking that it will allocate sane arrays. Then we might have a bad
+ time, and then compile that function with the FTL. The ByteCodeParser will represent the
+ allocation with a NewArray node that has a sane indexingType(). But when we go to lower the Node,
+ we observe that the Structure* that the JSGlobalObject tells us to use has a different indexing
+ type. This is a feature of havingABadTime that the DFG knew about, but the FTL didn't. The FTL
+ didn't know about it because we didn't have adequate tests, and this code rarely gets triggered in
+ the wild. So, the FTL had a silly assertion that the indexing types match. They absolutely don't
+ have to match.
+
+ There is another bug, a race condition, that remains even if we remove the bad assertion. We set
+ the havingABadTime watchpoint late in compilation, and we do it based on whether the watchpoint is
+ still OK. This means that we could parse a function before we have a bad time and then do
+ optimizations (for example in AbstractInterpreter) like proving that the structure set associated
+ with the value returned by the NewArray is the one with a sane indexing type. Then, after those
+ optimizations have already been done, we will go to set the watchpoint. But just as we are doing
+ this, we could haveABadTime on the main thread. Currently this sort of almost works because
+ having a bad time requires doing a GC, and we can't GC until the watchpoint collection phase. But
+ that feels too fragile. So, this phase moves the setting of the watchpoint to the FixupPhase. This
+ is consistent with our long-term goal of removing the WatchpointCollectionPhase. Moving this to
+ FixupPhase means that we set the watchpoint before doing any optimizations. So, if having a bad
+ time happens before the FixupPhase then all optimizations will agree that we're having a bad time
+ and so everything is fine; if we have a bad time after FixupPhase then we will cancel the
+ compilation anyway.
+
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::handleConstantInternalFunction):
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::fixupNode):
+ (JSC::DFG::FixupPhase::watchHavingABadTime):
+ (JSC::DFG::FixupPhase::createToString):
+ * dfg/DFGNode.h:
+ (JSC::DFG::Node::hasIndexingType):
+ (JSC::DFG::Node::indexingType):
+ * dfg/DFGWatchpointCollectionPhase.cpp:
+ (JSC::DFG::WatchpointCollectionPhase::handle):
+ * ftl/FTLLowerDFGToLLVM.cpp:
+ (JSC::FTL::DFG::LowerDFGToLLVM::compileNewArray):
+ (JSC::FTL::DFG::LowerDFGToLLVM::compileNewArrayBuffer):
+ * tests/stress/ftl-has-a-bad-time.js: Added.
+
</ins><span class="cx"> 2015-12-04 Benjamin Poulain <bpoulain@apple.com>
</span><span class="cx">
</span><span class="cx"> [JSC] Use Div and ChillDiv in FTL(B3)Output
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGByteCodeParsercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (193469 => 193470)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp 2015-12-04 22:15:23 UTC (rev 193469)
+++ trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp 2015-12-04 22:25:26 UTC (rev 193470)
</span><span class="lines">@@ -2418,13 +2418,6 @@
</span><span class="cx"> if (verbose)
</span><span class="cx"> dataLog(" Handling constant internal function ", JSValue(function), "\n");
</span><span class="cx">
</span><del>- // If we ever find that we have a lot of internal functions that we specialize for,
- // then we should probably have some sort of hashtable dispatch, or maybe even
- // dispatch straight through the MethodTable of the InternalFunction. But for now,
- // it seems that this case is hit infrequently enough, and the number of functions
- // we know about is small enough, that having just a linear cascade of if statements
- // is good enough.
-
</del><span class="cx"> if (function->classInfo() == ArrayConstructor::info()) {
</span><span class="cx"> if (function->globalObject() != m_inlineStackTop->m_codeBlock->globalObject())
</span><span class="cx"> return false;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGFixupPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp (193469 => 193470)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp 2015-12-04 22:15:23 UTC (rev 193469)
+++ trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp 2015-12-04 22:25:26 UTC (rev 193470)
</span><span class="lines">@@ -925,6 +925,8 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> case NewArray: {
</span><ins>+ watchHavingABadTime(node);
+
</ins><span class="cx"> for (unsigned i = m_graph.varArgNumChildren(node); i--;) {
</span><span class="cx"> node->setIndexingType(
</span><span class="cx"> leastUpperBoundOfIndexingTypeAndType(
</span><span class="lines">@@ -962,6 +964,8 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> case NewTypedArray: {
</span><ins>+ watchHavingABadTime(node);
+
</ins><span class="cx"> if (node->child1()->shouldSpeculateInt32()) {
</span><span class="cx"> fixEdge<Int32Use>(node->child1());
</span><span class="cx"> node->clearFlags(NodeMustGenerate);
</span><span class="lines">@@ -971,6 +975,7 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> case NewArrayWithSize: {
</span><ins>+ watchHavingABadTime(node);
</ins><span class="cx"> fixEdge<Int32Use>(node->child1());
</span><span class="cx"> break;
</span><span class="cx"> }
</span><span class="lines">@@ -1450,6 +1455,20 @@
</span><span class="cx"> #endif
</span><span class="cx"> }
</span><span class="cx"> }
</span><ins>+
+ void watchHavingABadTime(Node* node)
+ {
+ JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
+
+ // If this global object is not having a bad time, watch it. We go down this path anytime the code
+ // does an array allocation. The types of array allocations may change if we start to have a bad
+ // time. It's easier to reason about this if we know that whenever the types change after we start
+ // optimizing, the code just gets thrown out. Doing this at FixupPhase is just early enough, since
+ // prior to this point nobody should have been doing optimizations based on the indexing type of
+ // the allocation.
+ if (!globalObject->isHavingABadTime())
+ m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());
+ }
</ins><span class="cx">
</span><span class="cx"> template<UseKind useKind>
</span><span class="cx"> void createToString(Node* node, Edge& edge)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGNodeh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGNode.h (193469 => 193470)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGNode.h 2015-12-04 22:15:23 UTC (rev 193469)
+++ trunk/Source/JavaScriptCore/dfg/DFGNode.h 2015-12-04 22:25:26 UTC (rev 193470)
</span><span class="lines">@@ -949,7 +949,15 @@
</span><span class="cx"> return false;
</span><span class="cx"> }
</span><span class="cx"> }
</span><del>-
</del><ins>+
+ // Return the indexing type that an array allocation *wants* to use. It may end up using a different
+ // type if we're having a bad time. You can determine the actual indexing type by asking the global
+ // object:
+ //
+ // m_graph.globalObjectFor(node->origin.semantic)->arrayStructureForIndexingTypeDuringAllocation(node->indexingType())
+ //
+ // This will give you a Structure*, and that will have some indexing type that may be different from
+ // the this one.
</ins><span class="cx"> IndexingType indexingType()
</span><span class="cx"> {
</span><span class="cx"> ASSERT(hasIndexingType());
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGWatchpointCollectionPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGWatchpointCollectionPhase.cpp (193469 => 193470)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGWatchpointCollectionPhase.cpp 2015-12-04 22:15:23 UTC (rev 193469)
+++ trunk/Source/JavaScriptCore/dfg/DFGWatchpointCollectionPhase.cpp 2015-12-04 22:25:26 UTC (rev 193470)
</span><span class="lines">@@ -97,13 +97,6 @@
</span><span class="cx"> }
</span><span class="cx"> break;
</span><span class="cx">
</span><del>- case NewArray:
- case NewArrayWithSize:
- case NewArrayBuffer:
- if (!globalObject()->isHavingABadTime() && !hasAnyArrayStorage(m_node->indexingType()))
- addLazily(globalObject()->havingABadTimeWatchpoint());
- break;
-
</del><span class="cx"> case VarInjectionWatchpoint:
</span><span class="cx"> addLazily(globalObject()->varInjectionWatchpoint());
</span><span class="cx"> break;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreftlFTLLowerDFGToLLVMcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp (193469 => 193470)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp 2015-12-04 22:15:23 UTC (rev 193469)
+++ trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp 2015-12-04 22:25:26 UTC (rev 193470)
</span><span class="lines">@@ -3763,9 +3763,7 @@
</span><span class="cx"> JSGlobalObject* globalObject = m_graph.globalObjectFor(m_node->origin.semantic);
</span><span class="cx"> Structure* structure = globalObject->arrayStructureForIndexingTypeDuringAllocation(
</span><span class="cx"> m_node->indexingType());
</span><del>-
- DFG_ASSERT(m_graph, m_node, structure->indexingType() == m_node->indexingType());
-
</del><ins>+
</ins><span class="cx"> if (!globalObject->isHavingABadTime() && !hasAnyArrayStorage(m_node->indexingType())) {
</span><span class="cx"> unsigned numElements = m_node->numChildren();
</span><span class="cx">
</span><span class="lines">@@ -3842,8 +3840,6 @@
</span><span class="cx"> Structure* structure = globalObject->arrayStructureForIndexingTypeDuringAllocation(
</span><span class="cx"> m_node->indexingType());
</span><span class="cx">
</span><del>- DFG_ASSERT(m_graph, m_node, structure->indexingType() == m_node->indexingType());
-
</del><span class="cx"> if (!globalObject->isHavingABadTime() && !hasAnyArrayStorage(m_node->indexingType())) {
</span><span class="cx"> unsigned numElements = m_node->numConstants();
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>