Bug 152227 - [ES6] Handle new_generator_func / new_generator_func_exp in DFG / FTL
Summary: [ES6] Handle new_generator_func / new_generator_func_exp in DFG / FTL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on: 152333
Blocks: 151546
  Show dependency treegraph
 
Reported: 2015-12-13 04:42 PST by Yusuke Suzuki
Modified: 2015-12-17 02:33 PST (History)
6 users (show)

See Also:


Attachments
Patch (30.11 KB, patch)
2015-12-15 09:25 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (48.52 KB, patch)
2015-12-16 09:17 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-12-13 04:42:12 PST
...
Comment 1 Yusuke Suzuki 2015-12-15 09:25:06 PST
Created attachment 267375 [details]
Patch
Comment 2 Saam Barati 2015-12-15 09:36:29 PST
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 3 Keith Miller 2015-12-15 09:49:56 PST
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 4 Yusuke Suzuki 2015-12-15 09:52:35 PST
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 5 Yusuke Suzuki 2015-12-15 09:56:26 PST
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 6 Yusuke Suzuki 2015-12-15 09:59:06 PST
Comment on attachment 267375 [details]
Patch

And while implementing this, I found that NewArrowFunction is no longer used. I'm planning to drop this.
Comment 7 Yusuke Suzuki 2015-12-16 01:10:40 PST
Committed r194135: <http://trac.webkit.org/changeset/194135>
Comment 8 Csaba Osztrogonác 2015-12-16 03:13:38 PST
(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
Comment 9 Yusuke Suzuki 2015-12-16 03:50:52 PST
(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.
Comment 10 Yusuke Suzuki 2015-12-16 04:02:01 PST
I've figure out the issue.
On OSR, we should materialize JSGeneratorFunction in FTL.
Soon, it will be fixed.
Comment 11 Yusuke Suzuki 2015-12-16 04:08:24 PST
This involves ExitTimeObjectMaterialization, it may be non trivial.
So I'll roll back this patch now and upload the revised patch including ExitTimeObjectMaterialization change :)
Comment 12 WebKit Commit Bot 2015-12-16 04:45:05 PST
Re-opened since this is blocked by bug 152333
Comment 13 Yusuke Suzuki 2015-12-16 09:17:21 PST
Created attachment 267461 [details]
Patch
Comment 14 Saam Barati 2015-12-16 10:30:41 PST
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.
Comment 15 Yusuke Suzuki 2015-12-16 10:34:18 PST
(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.
Comment 16 Yusuke Suzuki 2015-12-16 11:54:08 PST
And perf improvement.

generator-function-create                     135.9831+-6.8084     ^      8.1685+-1.4068        ^ definitely 16.6472x faster
Comment 17 Yusuke Suzuki 2015-12-17 02:33:40 PST
Committed r194216: <http://trac.webkit.org/changeset/194216>