...
Created attachment 267375 [details] Patch
Comment on attachment 267375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267375&action=review r=me > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4581 > + if (opcodeID == op_new_func_exp || opcodeID == op_new_generator_func_exp) { > // Curly braces are necessary > + RELEASE_ASSERT(OPCODE_LENGTH(op_new_func_exp) == OPCODE_LENGTH(op_new_generator_func_exp)); > NEXT_OPCODE(op_new_func_exp); Couldn't we just replace this entire if/else statement with "NEXT_OPCODE(opcodeID)"? > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:142 > + enum class Kind { Escaped, Object, Activation, Function, ArrowFunction, GeneratorFunction }; Can you add a test where allocation sinking kicks in for NewGeneratorFunction? (Or do we already have one?) > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1461 > + allocation.kind() == Allocation::Kind::GeneratorFunction ? NewGeneratorFunction : NewFunction; style: extra space after "?"
Comment on attachment 267375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267375&action=review >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4581 >> NEXT_OPCODE(op_new_func_exp); > > Couldn't we just replace this entire if/else statement with "NEXT_OPCODE(opcodeID)"? You can't do that because it uses NEXT_OPCODE uses OPCODE_LENGTH which appends _length to the name you provide. I think the RELEASE_ASSERT can be a static_assert, however.
Comment on attachment 267375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267375&action=review Thank you for your review! >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4581 >> NEXT_OPCODE(op_new_func_exp); > > Couldn't we just replace this entire if/else statement with "NEXT_OPCODE(opcodeID)"? We cannot do that. NEXT_OPCODE is defined as follows. 1126 #define NEXT_OPCODE(name) \ 1127 m_currentIndex += OPCODE_LENGTH(name); \ 1128 continue And OPCODE_LENGTH is defined as follows. 72 #define OPCODE_LENGTH(opcode) opcode##_length So the argument of NEXT_OPCODE need to be opcode token. >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:142 >> + enum class Kind { Escaped, Object, Activation, Function, ArrowFunction, GeneratorFunction }; > > Can you add a test where allocation sinking kicks in for NewGeneratorFunction? (Or do we already have one?) Nice, I've just added tests; copying the tests for function allocation sinking, and change it to generator function, and check this is a generator function. >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1461 >> + allocation.kind() == Allocation::Kind::GeneratorFunction ? NewGeneratorFunction : NewFunction; > > style: extra space after "?" Oooops! Thank you! Dropped.
Comment on attachment 267375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267375&action=review >>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4581 >>>> NEXT_OPCODE(op_new_func_exp); >>> >>> Couldn't we just replace this entire if/else statement with "NEXT_OPCODE(opcodeID)"? >> >> You can't do that because it uses NEXT_OPCODE uses OPCODE_LENGTH which appends _length to the name you provide. I think the RELEASE_ASSERT can be a static_assert, however. > > We cannot do that. > > NEXT_OPCODE is defined as follows. > > 1126 #define NEXT_OPCODE(name) \ > 1127 m_currentIndex += OPCODE_LENGTH(name); \ > 1128 continue > > And OPCODE_LENGTH is defined as follows. > > 72 #define OPCODE_LENGTH(opcode) opcode##_length > > So the argument of NEXT_OPCODE need to be opcode token. Nice catch! We can perform static_assert here :D Changed.
Comment on attachment 267375 [details] Patch And while implementing this, I found that NewArrowFunction is no longer used. I'm planning to drop this.
Committed r194135: <http://trac.webkit.org/changeset/194135>
(In reply to comment #7) > Committed r194135: <http://trac.webkit.org/changeset/194135> The following new tests fail everywhere and made bots red: stress/generator-function-declaration-sinking-osrexit.js stress/generator-function-expression-sinking-osrexit.js
(In reply to comment #8) > (In reply to comment #7) > > Committed r194135: <http://trac.webkit.org/changeset/194135> > > The following new tests fail everywhere and made bots red: > stress/generator-function-declaration-sinking-osrexit.js > stress/generator-function-expression-sinking-osrexit.js Oops, I'll investigate and fix it soon.
I've figure out the issue. On OSR, we should materialize JSGeneratorFunction in FTL. Soon, it will be fixed.
This involves ExitTimeObjectMaterialization, it may be non trivial. So I'll roll back this patch now and upload the revised patch including ExitTimeObjectMaterialization change :)
Re-opened since this is blocked by bug 152333
Created attachment 267461 [details] Patch
Comment on attachment 267461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267461&action=review r=me > Source/JavaScriptCore/ftl/FTLOperations.cpp:180 > + if (materialization->type() == PhantomNewFunction) > + return JSFunction::createWithInvalidatedReallocationWatchpoint(vm, executable, activation); It seems like we might sink an arrow function and end up not reallocating an arrow function, but a JSFunction instead. I think that's fine for now b/c we're going to remove JSArrowFunction, but it's definitely a bug.
(In reply to comment #14) > Comment on attachment 267461 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267461&action=review > > r=me > > > Source/JavaScriptCore/ftl/FTLOperations.cpp:180 > > + if (materialization->type() == PhantomNewFunction) > > + return JSFunction::createWithInvalidatedReallocationWatchpoint(vm, executable, activation); > > It seems like we might sink an arrow function and end up not reallocating an > arrow function, but a JSFunction instead. > I think that's fine for now b/c we're going to remove JSArrowFunction, but > it's definitely a bug. Yup. This part for JSArrowFunction seems removed when landing http://trac.webkit.org/changeset/193766. So if we don't use NewArrowFunction further, we should remove all the NewArrowFunction part. But if not, we should not forget to add this part when reverting http://trac.webkit.org/changeset/193766 change.
And perf improvement. generator-function-create 135.9831+-6.8084 ^ 8.1685+-1.4068 ^ definitely 16.6472x faster
Committed r194216: <http://trac.webkit.org/changeset/194216>