Bug 152227 – [ES6] Handle new_generator_func / new_generator_func_exp in DFG / FTL

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 267375 [details]
Patch

[Saam Barati]

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 "?"

[Keith Miller]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Comment on attachment 267375 [details]
Patch

And while implementing this, I found that NewArrowFunction is no longer used. I'm planning to drop this.

[Yusuke Suzuki]

Committed r194135: <http://trac.webkit.org/changeset/194135>

[Csaba Osztrogonác]

(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

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

I've figure out the issue.
On OSR, we should materialize JSGeneratorFunction in FTL.
Soon, it will be fixed.

[Yusuke Suzuki]

This involves ExitTimeObjectMaterialization, it may be non trivial.
So I'll roll back this patch now and upload the revised patch including ExitTimeObjectMaterialization change :)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 152333

[Yusuke Suzuki]

Created attachment 267461 [details]
Patch

[Saam Barati]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

And perf improvement.

generator-function-create                     135.9831+-6.8084     ^      8.1685+-1.4068        ^ definitely 16.6472x faster

[Yusuke Suzuki]

Committed r194216: <http://trac.webkit.org/changeset/194216>