RegExpExec/RegExpTest should not unconditionally speculate cell
https://bugs.webkit.org/show_bug.cgi?id=154901
Reviewed by Benjamin Poulain.
This is a three part change. It all started with a simple goal: end the rage-recompiles in
Octane/regexp by enabling the DFG and FTL to do untyped RegExpExec/RegExpTest. This keeps us
in the optimized code when you do a regexp match on a number, for example.
While implementing this, I realized that DFGOperations.cpp was bad at exception checking. When
it did check for exceptions, it used exec->hadException() instead of vm.exception(). So I
fixed that. I also made sure that the regexp operations checked for exception after doing
toString().
Unfortunately, the introduction of untyped RegExpExec/RegExpTest caused a regression on
Octane/regexp. This was because we were simultaneously scheduling replacement and OSR compiles
of some large functions with the FTL JIT. The OSR compiles were not useful. This was a
regression from the previous changes to make OSR compiles happen sooner. The problem is that
this change also removed the throttling of OSR compiles even in those cases where we suspect
that replacement is more likely. This patch reintroduces that throttling, but only in the
replacement path.
This change ends up being neutral overall.
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGOperations.cpp:
* dfg/DFGOperations.h:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileRegExpExec):
(JSC::FTL::DFG::LowerDFGToB3::compileRegExpTest):
(JSC::FTL::DFG::LowerDFGToB3::compileNewRegexp):
* tests/stress/regexp-exec-effect-after-exception.js: Added.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@197492 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 931d67f..9338e5e 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,43 @@
+2016-03-02 Filip Pizlo <fpizlo@apple.com>
+
+ RegExpExec/RegExpTest should not unconditionally speculate cell
+ https://bugs.webkit.org/show_bug.cgi?id=154901
+
+ Reviewed by Benjamin Poulain.
+
+ This is a three part change. It all started with a simple goal: end the rage-recompiles in
+ Octane/regexp by enabling the DFG and FTL to do untyped RegExpExec/RegExpTest. This keeps us
+ in the optimized code when you do a regexp match on a number, for example.
+
+ While implementing this, I realized that DFGOperations.cpp was bad at exception checking. When
+ it did check for exceptions, it used exec->hadException() instead of vm.exception(). So I
+ fixed that. I also made sure that the regexp operations checked for exception after doing
+ toString().
+
+ Unfortunately, the introduction of untyped RegExpExec/RegExpTest caused a regression on
+ Octane/regexp. This was because we were simultaneously scheduling replacement and OSR compiles
+ of some large functions with the FTL JIT. The OSR compiles were not useful. This was a
+ regression from the previous changes to make OSR compiles happen sooner. The problem is that
+ this change also removed the throttling of OSR compiles even in those cases where we suspect
+ that replacement is more likely. This patch reintroduces that throttling, but only in the
+ replacement path.
+
+ This change ends up being neutral overall.
+
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::fixupNode):
+ * dfg/DFGOperations.cpp:
+ * dfg/DFGOperations.h:
+ * dfg/DFGSpeculativeJIT32_64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileRegExpExec):
+ (JSC::FTL::DFG::LowerDFGToB3::compileRegExpTest):
+ (JSC::FTL::DFG::LowerDFGToB3::compileNewRegexp):
+ * tests/stress/regexp-exec-effect-after-exception.js: Added.
+
2016-03-02 Benjamin Poulain <bpoulain@apple.com>
[JSC] JSCell_freeListNext and JSCell_structureID are considered not overlapping
diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index af86a46..6b83dcb 100644
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -879,8 +879,14 @@
case RegExpExec:
case RegExpTest: {
- fixEdge<CellUse>(node->child1());
- fixEdge<CellUse>(node->child2());
+ // FIXME: These should probably speculate something stronger than cell.
+ // https://bugs.webkit.org/show_bug.cgi?id=154900
+ if (node->child1()->shouldSpeculateCell()
+ && node->child2()->shouldSpeculateCell()) {
+ fixEdge<CellUse>(node->child1());
+ fixEdge<CellUse>(node->child2());
+ break;
+ }
break;
}
diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
index 942b4ad..b663bdc 100644
--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
@@ -361,10 +361,10 @@
}
baseValue.requireObjectCoercible(exec);
- if (exec->hadException())
+ if (vm.exception())
return JSValue::encode(jsUndefined());
auto propertyName = property.toPropertyKey(exec);
- if (exec->hadException())
+ if (vm.exception())
return JSValue::encode(jsUndefined());
return JSValue::encode(baseValue.get(exec, propertyName));
}
@@ -394,7 +394,7 @@
}
auto propertyName = property.toPropertyKey(exec);
- if (exec->hadException())
+ if (vm.exception())
return JSValue::encode(jsUndefined());
return JSValue::encode(JSValue(base).get(exec, propertyName));
}
@@ -622,8 +622,31 @@
if (!base->inherits(RegExpObject::info()))
return throwVMTypeError(exec);
- ASSERT(argument->isString() || argument->isObject());
- JSString* input = argument->isString() ? asString(argument) : asObject(argument)->toString(exec);
+ JSString* input;
+ if (argument->isString())
+ input = asString(argument);
+ else {
+ input = JSValue(argument).toStringOrNull(exec);
+ if (!input)
+ return JSValue::encode(jsUndefined());
+ }
+ return JSValue::encode(asRegExpObject(base)->exec(exec, input));
+}
+
+EncodedJSValue JIT_OPERATION operationRegExpExecGeneric(ExecState* exec, EncodedJSValue encodedBase, EncodedJSValue encodedArgument)
+{
+ VM& vm = exec->vm();
+ NativeCallFrameTracer tracer(&vm, exec);
+
+ JSValue base = JSValue::decode(encodedBase);
+ JSValue argument = JSValue::decode(encodedArgument);
+
+ if (!base.inherits(RegExpObject::info()))
+ return throwVMTypeError(exec);
+
+ JSString* input = argument.toStringOrNull(exec);
+ if (!input)
+ return JSValue::encode(jsUndefined());
return JSValue::encode(asRegExpObject(base)->exec(exec, input));
}
@@ -637,8 +660,33 @@
return false;
}
- ASSERT(argument->isString() || argument->isObject());
- JSString* input = argument->isString() ? asString(argument) : asObject(argument)->toString(exec);
+ JSString* input;
+ if (argument->isString())
+ input = asString(argument);
+ else {
+ input = JSValue(argument).toStringOrNull(exec);
+ if (!input)
+ return false;
+ }
+ return asRegExpObject(base)->test(exec, input);
+}
+
+size_t JIT_OPERATION operationRegExpTestGeneric(ExecState* exec, EncodedJSValue encodedBase, EncodedJSValue encodedArgument)
+{
+ VM& vm = exec->vm();
+ NativeCallFrameTracer tracer(&vm, exec);
+
+ JSValue base = JSValue::decode(encodedBase);
+ JSValue argument = JSValue::decode(encodedArgument);
+
+ if (!base.inherits(RegExpObject::info())) {
+ throwTypeError(exec);
+ return false;
+ }
+
+ JSString* input = argument.toStringOrNull(exec);
+ if (!input)
+ return false;
return asRegExpObject(base)->test(exec, input);
}
@@ -1182,9 +1230,9 @@
NativeCallFrameTracer tracer(&vm, exec);
JSString* str1 = JSValue::decode(a).toString(exec);
- ASSERT(!exec->hadException()); // Impossible, since we must have been given primitives.
+ ASSERT(!vm.exception()); // Impossible, since we must have been given primitives.
JSString* str2 = JSValue::decode(b).toString(exec);
- ASSERT(!exec->hadException());
+ ASSERT(!vm.exception());
if (sumOverflows<int32_t>(str1->length(), str2->length())) {
throwOutOfMemoryError(exec);
@@ -1200,11 +1248,11 @@
NativeCallFrameTracer tracer(&vm, exec);
JSString* str1 = JSValue::decode(a).toString(exec);
- ASSERT(!exec->hadException()); // Impossible, since we must have been given primitives.
+ ASSERT(!vm.exception()); // Impossible, since we must have been given primitives.
JSString* str2 = JSValue::decode(b).toString(exec);
- ASSERT(!exec->hadException());
+ ASSERT(!vm.exception());
JSString* str3 = JSValue::decode(c).toString(exec);
- ASSERT(!exec->hadException());
+ ASSERT(!vm.exception());
if (sumOverflows<int32_t>(str1->length(), str2->length(), str3->length())) {
throwOutOfMemoryError(exec);
@@ -1560,6 +1608,11 @@
if (!codeBlock->hasOptimizedReplacement())
return nullptr;
+
+ if (jitCode->osrEntryRetry < Options::ftlOSREntryRetryThreshold()) {
+ jitCode->osrEntryRetry++;
+ return nullptr;
+ }
}
// It's time to try to compile code for OSR entry.
diff --git a/Source/JavaScriptCore/dfg/DFGOperations.h b/Source/JavaScriptCore/dfg/DFGOperations.h
index 409629f..ca6424b 100644
--- a/Source/JavaScriptCore/dfg/DFGOperations.h
+++ b/Source/JavaScriptCore/dfg/DFGOperations.h
@@ -102,8 +102,10 @@
EncodedJSValue JIT_OPERATION operationArrayPop(ExecState*, JSArray*) WTF_INTERNAL;
EncodedJSValue JIT_OPERATION operationArrayPopAndRecoverLength(ExecState*, JSArray*) WTF_INTERNAL;
EncodedJSValue JIT_OPERATION operationRegExpExec(ExecState*, JSCell*, JSCell*) WTF_INTERNAL;
+EncodedJSValue JIT_OPERATION operationRegExpExecGeneric(ExecState*, EncodedJSValue, EncodedJSValue) WTF_INTERNAL;
// These comparisons return a boolean within a size_t such that the value is zero extended to fill the register.
size_t JIT_OPERATION operationRegExpTest(ExecState*, JSCell*, JSCell*) WTF_INTERNAL;
+size_t JIT_OPERATION operationRegExpTestGeneric(ExecState*, EncodedJSValue, EncodedJSValue) WTF_INTERNAL;
size_t JIT_OPERATION operationCompareStrictEqCell(ExecState*, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2) WTF_INTERNAL;
size_t JIT_OPERATION operationCompareStrictEq(ExecState*, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2) WTF_INTERNAL;
JSCell* JIT_OPERATION operationCreateActivationDirect(ExecState*, Structure*, JSScope*, SymbolTable*, EncodedJSValue);
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
index e39237b..25b92d7 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
@@ -2836,15 +2836,34 @@
}
case RegExpExec: {
- SpeculateCellOperand base(this, node->child1());
- SpeculateCellOperand argument(this, node->child2());
- GPRReg baseGPR = base.gpr();
- GPRReg argumentGPR = argument.gpr();
+ if (node->child1().useKind() == CellUse
+ && node->child2().useKind() == CellUse) {
+ SpeculateCellOperand base(this, node->child1());
+ SpeculateCellOperand argument(this, node->child2());
+ GPRReg baseGPR = base.gpr();
+ GPRReg argumentGPR = argument.gpr();
+
+ flushRegisters();
+ GPRFlushedCallResult2 resultTag(this);
+ GPRFlushedCallResult resultPayload(this);
+ callOperation(operationRegExpExec, resultTag.gpr(), resultPayload.gpr(), baseGPR, argumentGPR);
+ m_jit.exceptionCheck();
+
+ jsValueResult(resultTag.gpr(), resultPayload.gpr(), node);
+ break;
+ }
+
+ JSValueOperand base(this, node->child1());
+ JSValueOperand argument(this, node->child2());
+ GPRReg baseTagGPR = base.tagGPR();
+ GPRReg basePayloadGPR = base.payloadGPR();
+ GPRReg argumentTagGPR = argument.tagGPR();
+ GPRReg argumentPayloadGPR = argument.payloadGPR();
flushRegisters();
GPRFlushedCallResult2 resultTag(this);
GPRFlushedCallResult resultPayload(this);
- callOperation(operationRegExpExec, resultTag.gpr(), resultPayload.gpr(), baseGPR, argumentGPR);
+ callOperation(operationRegExpExecGeneric, resultTag.gpr(), resultPayload.gpr(), baseTagGPR, basePayloadGPR, argumentTagGPR, argumentPayloadGPR);
m_jit.exceptionCheck();
jsValueResult(resultTag.gpr(), resultPayload.gpr(), node);
@@ -2852,17 +2871,34 @@
}
case RegExpTest: {
- SpeculateCellOperand base(this, node->child1());
- SpeculateCellOperand argument(this, node->child2());
- GPRReg baseGPR = base.gpr();
- GPRReg argumentGPR = argument.gpr();
+ if (node->child1().useKind() == CellUse
+ && node->child2().useKind() == CellUse) {
+ SpeculateCellOperand base(this, node->child1());
+ SpeculateCellOperand argument(this, node->child2());
+ GPRReg baseGPR = base.gpr();
+ GPRReg argumentGPR = argument.gpr();
+
+ flushRegisters();
+ GPRFlushedCallResult result(this);
+ callOperation(operationRegExpTest, result.gpr(), baseGPR, argumentGPR);
+ m_jit.exceptionCheck();
+
+ booleanResult(result.gpr(), node);
+ break;
+ }
+
+ JSValueOperand base(this, node->child1());
+ JSValueOperand argument(this, node->child2());
+ GPRReg baseTagGPR = base.tagGPR();
+ GPRReg basePayloadGPR = base.payloadGPR();
+ GPRReg argumentTagGPR = argument.tagGPR();
+ GPRReg argumentPayloadGPR = argument.payloadGPR();
flushRegisters();
GPRFlushedCallResult result(this);
- callOperation(operationRegExpTest, result.gpr(), baseGPR, argumentGPR);
+ callOperation(operationRegExpTestGeneric, result.gpr(), baseTagGPR, basePayloadGPR, argumentTagGPR, argumentPayloadGPR);
m_jit.exceptionCheck();
- // If we add a DataFormatBool, we should use it here.
booleanResult(result.gpr(), node);
break;
}
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
index 06b4008..199009d 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
@@ -2961,14 +2961,30 @@
}
case RegExpExec: {
- SpeculateCellOperand base(this, node->child1());
- SpeculateCellOperand argument(this, node->child2());
+ if (node->child1().useKind() == CellUse
+ && node->child2().useKind() == CellUse) {
+ SpeculateCellOperand base(this, node->child1());
+ SpeculateCellOperand argument(this, node->child2());
+ GPRReg baseGPR = base.gpr();
+ GPRReg argumentGPR = argument.gpr();
+
+ flushRegisters();
+ GPRFlushedCallResult result(this);
+ callOperation(operationRegExpExec, result.gpr(), baseGPR, argumentGPR);
+ m_jit.exceptionCheck();
+
+ jsValueResult(result.gpr(), node);
+ break;
+ }
+
+ JSValueOperand base(this, node->child1());
+ JSValueOperand argument(this, node->child2());
GPRReg baseGPR = base.gpr();
GPRReg argumentGPR = argument.gpr();
flushRegisters();
GPRFlushedCallResult result(this);
- callOperation(operationRegExpExec, result.gpr(), baseGPR, argumentGPR);
+ callOperation(operationRegExpExecGeneric, result.gpr(), baseGPR, argumentGPR);
m_jit.exceptionCheck();
jsValueResult(result.gpr(), node);
@@ -2976,14 +2992,32 @@
}
case RegExpTest: {
- SpeculateCellOperand base(this, node->child1());
- SpeculateCellOperand argument(this, node->child2());
+ if (node->child1().useKind() == CellUse
+ && node->child2().useKind() == CellUse) {
+ SpeculateCellOperand base(this, node->child1());
+ SpeculateCellOperand argument(this, node->child2());
+ GPRReg baseGPR = base.gpr();
+ GPRReg argumentGPR = argument.gpr();
+
+ flushRegisters();
+ GPRFlushedCallResult result(this);
+ callOperation(operationRegExpTest, result.gpr(), baseGPR, argumentGPR);
+ m_jit.exceptionCheck();
+
+ // If we add a DataFormatBool, we should use it here.
+ m_jit.or32(TrustedImm32(ValueFalse), result.gpr());
+ jsValueResult(result.gpr(), node, DataFormatJSBoolean);
+ break;
+ }
+
+ JSValueOperand base(this, node->child1());
+ JSValueOperand argument(this, node->child2());
GPRReg baseGPR = base.gpr();
GPRReg argumentGPR = argument.gpr();
flushRegisters();
GPRFlushedCallResult result(this);
- callOperation(operationRegExpTest, result.gpr(), baseGPR, argumentGPR);
+ callOperation(operationRegExpTestGeneric, result.gpr(), baseGPR, argumentGPR);
m_jit.exceptionCheck();
// If we add a DataFormatBool, we should use it here.
diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
index a25f6a1..512f6da 100644
--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
@@ -6434,18 +6434,36 @@
void compileRegExpExec()
{
- LValue base = lowCell(m_node->child1());
- LValue argument = lowCell(m_node->child2());
+ if (m_node->child1().useKind() == CellUse
+ && m_node->child2().useKind() == CellUse) {
+ LValue base = lowCell(m_node->child1());
+ LValue argument = lowCell(m_node->child2());
+ setJSValue(
+ vmCall(Int64, m_out.operation(operationRegExpExec), m_callFrame, base, argument));
+ return;
+ }
+
+ LValue base = lowJSValue(m_node->child1());
+ LValue argument = lowJSValue(m_node->child2());
setJSValue(
- vmCall(Int64, m_out.operation(operationRegExpExec), m_callFrame, base, argument));
+ vmCall(Int64, m_out.operation(operationRegExpExecGeneric), m_callFrame, base, argument));
}
void compileRegExpTest()
{
- LValue base = lowCell(m_node->child1());
- LValue argument = lowCell(m_node->child2());
+ if (m_node->child1().useKind() == CellUse
+ && m_node->child2().useKind() == CellUse) {
+ LValue base = lowCell(m_node->child1());
+ LValue argument = lowCell(m_node->child2());
+ setBoolean(
+ vmCall(Int32, m_out.operation(operationRegExpTest), m_callFrame, base, argument));
+ return;
+ }
+
+ LValue base = lowJSValue(m_node->child1());
+ LValue argument = lowJSValue(m_node->child2());
setBoolean(
- vmCall(Int32, m_out.operation(operationRegExpTest), m_callFrame, base, argument));
+ vmCall(Int32, m_out.operation(operationRegExpTestGeneric), m_callFrame, base, argument));
}
void compileNewRegexp()
diff --git a/Source/JavaScriptCore/tests/stress/regexp-exec-effect-after-exception.js b/Source/JavaScriptCore/tests/stress/regexp-exec-effect-after-exception.js
new file mode 100644
index 0000000..a737429
--- /dev/null
+++ b/Source/JavaScriptCore/tests/stress/regexp-exec-effect-after-exception.js
@@ -0,0 +1,26 @@
+function foo(s) {
+ return /.*/.exec(s);
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i)
+ foo("foo bar");
+
+RegExp.input = "blah";
+
+var didFinish = false;
+try {
+ foo({toString: function() {
+ throw "error";
+ }});
+ didFinish = true;
+} catch (e) {
+ if (e != "error")
+ throw "Error: bad exception at end: " + e;
+ if (RegExp.input != "blah")
+ throw "Error: bad value of input: " + RegExp.input;
+}
+
+if (didFinish)
+ throw "Error: did not throw exception.";
