Title: [295658] trunk
- Revision
- 295658
- Author
- ysuz...@apple.com
- Date
- 2022-06-17 23:46:29 -0700 (Fri, 17 Jun 2022)
Log Message
[JSC] Fix iterator_next's tmp liveness and OSR exit recovery
https://bugs.webkit.org/show_bug.cgi?id=241702
Reviewed by Mark Lam.
We fix two issues in iterator_next DFG handling.
1. Consider the following case,
function inlinedGetterUsedByIteratorNext()
{
if (flag)
ForceOSRExit() // Terminal
...
}
And we hit ForceOSRExit and do OSR exit. We are not reporting tmp (nextResult tmp in this case) as live at
the terminal accidentally. As a result, when OSR exit is performed, it is dead.
But this is still used after "done" lookup is finished since "value" lookup also uses this nextResult. As
a result, we encounter an error since nextResult is not recovered after OSR exit.
In this patch, we report liveness of tmp in flushForTerminalImpl to recover them. Strictly speaking, this
code is slightly too conservative: for example, when OSR exit happens for inlined call of "value" getter, "value"'s
requiring tmp is not necessary since this is the last checkpoint and this llint_slow_path_checkpoint_osr_exit_from_inlined_call
is called after finishing the call => we finished all the things. For now, we align it to the other places since
this is conservatively correct. In a future patch, we can make it more precisely modeled.
2. llint_slow_path_checkpoint_osr_exit_from_inlined_call should not use handleIteratorNextCheckpoint
handleIteratorNextCheckpoint is not for inlined call. Inlined call is "OSR exit during the checkpoint's call".
Thus, its checkpoint meaning is different from llint_slow_path_checkpoint_osr_exit: for example, when OSR exit
happens for inlined call of "value" getter, all the operation is already done and only thing we need to do is
storing the result value to the specified VirtualRegister position. On the other hand, in llint_slow_path_checkpoint_osr_exit,
we should perform what we need to do in the last checkpoint sequence.
This patch fixes iterator_next's definition in llint_slow_path_checkpoint_osr_exit_from_inlined_call since it
is the only incorrect case.
* JSTests/stress/osr-exit-iterator-next-get-by-id-value-access.js: Added.
(result.get value):
(result.get done):
(iterator.next):
(object.Symbol.iterator):
(test):
* JSTests/stress/osr-exit-iterator-next-get-by-id-value-exit.js: Added.
(result.get value):
(result.get done):
(iterator.next):
(object.Symbol.iterator):
(test):
* JSTests/stress/osr-exit-iterator-next-get-by-id.js: Added.
(result.get value):
(result.get done):
(iterator.next):
(object.Symbol.iterator):
(test):
* JSTests/stress/osr-exit-iterator-open-get-by-id.js: Added.
(iterator.nextImpl):
(iterator.get next):
(object.Symbol.iterator):
(test):
* Source/_javascript_Core/dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::flushForTerminalImpl):
* Source/_javascript_Core/dfg/DFGOSRExitCompilerCommon.cpp:
(JSC::DFG::callerReturnPC):
(JSC::DFG::reifyInlinedCallFrames):
* Source/_javascript_Core/llint/LLIntSlowPaths.cpp:
(JSC::LLInt::handleIteratorNextCheckpoint):
(JSC::LLInt::llint_slow_path_checkpoint_osr_exit_from_inlined_call):
Canonical link: https://commits.webkit.org/251663@main
Modified Paths
Added Paths
Diff
Added: trunk/JSTests/stress/osr-exit-iterator-next-get-by-id-value-access.js (0 => 295658)
--- trunk/JSTests/stress/osr-exit-iterator-next-get-by-id-value-access.js (rev 0)
+++ trunk/JSTests/stress/osr-exit-iterator-next-get-by-id-value-access.js 2022-06-18 06:46:29 UTC (rev 295658)
@@ -0,0 +1,41 @@
+var flag = 0;
+var counter = 0;
+
+var result = {
+ get value() {
+ return 42;
+ },
+ get done() {
+ if (flag)
+ OSRExit();
+ ++counter
+ return counter & 0x1;
+ },
+};
+
+var iterator = {
+ next() {
+ return result;
+ }
+};
+
+
+var object = {
+ [Symbol.iterator]() {
+ return iterator;
+ }
+};
+
+noDFG(Object.getOwnPropertyDescriptor(object, Symbol.iterator).value);
+
+function test()
+{
+ for (let i of object);
+}
+noInline(test);
+
+for (var i = 0; i < 1e6; ++i)
+ test();
+flag = 1;
+for (var i = 0; i < 1e6; ++i)
+ test();
Added: trunk/JSTests/stress/osr-exit-iterator-next-get-by-id-value-exit.js (0 => 295658)
--- trunk/JSTests/stress/osr-exit-iterator-next-get-by-id-value-exit.js (rev 0)
+++ trunk/JSTests/stress/osr-exit-iterator-next-get-by-id-value-exit.js 2022-06-18 06:46:29 UTC (rev 295658)
@@ -0,0 +1,41 @@
+var flag = 0;
+var counter = 0;
+
+var result = {
+ get value() {
+ if (flag)
+ OSRExit();
+ return 42;
+ },
+ get done() {
+ ++counter
+ return counter & 0x1;
+ },
+};
+
+var iterator = {
+ next() {
+ return result;
+ }
+};
+
+
+var object = {
+ [Symbol.iterator]() {
+ return iterator;
+ }
+};
+
+noDFG(Object.getOwnPropertyDescriptor(object, Symbol.iterator).value);
+
+function test()
+{
+ for (let i of object);
+}
+noInline(test);
+
+for (var i = 0; i < 1e6; ++i)
+ test();
+flag = 1;
+for (var i = 0; i < 1e6; ++i)
+ test();
Added: trunk/JSTests/stress/osr-exit-iterator-next-get-by-id.js (0 => 295658)
--- trunk/JSTests/stress/osr-exit-iterator-next-get-by-id.js (rev 0)
+++ trunk/JSTests/stress/osr-exit-iterator-next-get-by-id.js 2022-06-18 06:46:29 UTC (rev 295658)
@@ -0,0 +1,39 @@
+var flag = 0;
+
+var result = {
+ get value() {
+ return 42;
+ },
+ get done() {
+ if (flag)
+ OSRExit();
+ return true;
+ },
+};
+
+var iterator = {
+ next() {
+ return result;
+ }
+};
+
+
+var object = {
+ [Symbol.iterator]() {
+ return iterator;
+ }
+};
+
+noDFG(Object.getOwnPropertyDescriptor(object, Symbol.iterator).value);
+
+function test()
+{
+ for (let i of object);
+}
+noInline(test);
+
+for (var i = 0; i < 1e6; ++i)
+ test();
+flag = 1;
+for (var i = 0; i < 1e6; ++i)
+ test();
Added: trunk/JSTests/stress/osr-exit-iterator-open-get-by-id.js (0 => 295658)
--- trunk/JSTests/stress/osr-exit-iterator-open-get-by-id.js (rev 0)
+++ trunk/JSTests/stress/osr-exit-iterator-open-get-by-id.js 2022-06-18 06:46:29 UTC (rev 295658)
@@ -0,0 +1,35 @@
+var flag = 0;
+flag = 1;
+flag = 0;
+var iterator = {
+ nextImpl() {
+ return { value: 42, done: true };
+ },
+
+ get next() {
+ if (flag)
+ OSRExit();
+ return this.nextImpl;
+ }
+};
+
+
+var object = {
+ [Symbol.iterator]() {
+ return iterator;
+ }
+};
+
+noDFG(Object.getOwnPropertyDescriptor(object, Symbol.iterator).value);
+
+function test()
+{
+ for (let i of object);
+}
+noInline(test);
+
+for (var i = 0; i < 1e6; ++i)
+ test();
+flag = 1;
+for (var i = 0; i < 1e6; ++i)
+ test();
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (295657 => 295658)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2022-06-18 05:54:10 UTC (rev 295657)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2022-06-18 06:46:29 UTC (rev 295658)
@@ -634,6 +634,13 @@
if (livenessAtBytecode[local])
addPhantomLocalDirect(inlineCallFrame, remapOperand(inlineCallFrame, virtualRegisterForLocal(local)));
}
+ if (bytecodeIndex.checkpoint()) {
+ ASSERT(codeBlock->numTmps());
+ auto liveTmps = tmpLivenessForCheckpoint(*codeBlock, bytecodeIndex);
+ liveTmps.forEachSetBit([&](size_t tmp) {
+ addPhantomLocalDirect(inlineCallFrame, remapOperand(inlineCallFrame, Operand::tmp(tmp)));
+ });
+ }
isCallerOrigin = true;
});
}
Modified: trunk/Source/_javascript_Core/llint/LLIntSlowPaths.cpp (295657 => 295658)
--- trunk/Source/_javascript_Core/llint/LLIntSlowPaths.cpp 2022-06-18 05:54:10 UTC (rev 295657)
+++ trunk/Source/_javascript_Core/llint/LLIntSlowPaths.cpp 2022-06-18 06:46:29 UTC (rev 295658)
@@ -2353,7 +2353,6 @@
auto scope = DECLARE_THROW_SCOPE(vm);
unsigned checkpointIndex = sideState.bytecodeIndex.checkpoint();
- auto& valueRegister = callFrame->uncheckedR(bytecode.m_value);
auto iteratorResultObject = sideState.tmps[OpIteratorNext::nextResult];
auto next = callFrame->uncheckedR(bytecode.m_next).jsValue();
RELEASE_ASSERT_WITH_MESSAGE(next, "We should not OSR exit to a checkpoint for fast cases.");
@@ -2370,6 +2369,7 @@
}
scope.release();
+ auto& valueRegister = callFrame->uncheckedR(bytecode.m_value);
if (doneRegister.jsValue().toBoolean(globalObject))
valueRegister = jsUndefined();
else
@@ -2433,11 +2433,25 @@
}
case op_iterator_next: {
callFrame->uncheckedR(destinationFor(pc->as<OpIteratorNext>(), bytecodeIndex.checkpoint()).virtualRegister()) = JSValue::decode(result);
- if (bytecodeIndex.checkpoint() == OpIteratorNext::getValue)
+ switch (bytecodeIndex.checkpoint()) {
+ case OpIteratorNext::computeNext: // First one is not handled by checkpoint.
+ RELEASE_ASSERT_NOT_REACHED();
break;
- ASSERT(bytecodeIndex.checkpoint() == OpIteratorNext::getDone);
- sideState->bytecodeIndex = bytecodeIndex.withCheckpoint(OpIteratorNext::getValue);
- handleIteratorNextCheckpoint(vm, callFrame, globalObject, pc->as<OpIteratorNext>(), *sideState.get());
+ case OpIteratorNext::getDone: {
+ const auto& bytecode = pc->as<OpIteratorNext>();
+ auto& doneRegister = callFrame->uncheckedR(bytecode.m_done);
+ auto& valueRegister = callFrame->uncheckedR(bytecode.m_value);
+ if (doneRegister.jsValue().toBoolean(globalObject))
+ valueRegister = jsUndefined();
+ else {
+ auto iteratorResultObject = sideState->tmps[OpIteratorNext::nextResult];
+ valueRegister = iteratorResultObject.get(globalObject, vm.propertyNames->value);
+ }
+ break;
+ }
+ case OpIteratorNext::getValue:
+ break;
+ }
break;
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
