Bug 153981 - [ES6] Arrow function syntax. Emit loading&putting this/super only if they are used in arrow function
Summary: [ES6] Arrow function syntax. Emit loading&putting this/super only if they are...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 149933 (view as bug list)
Depends on: 154649
Blocks: 140855
  Show dependency treegraph
 
Reported: 2016-02-08 03:25 PST by GSkachkov
Modified: 2017-08-20 00:52 PDT (History)
13 users (show)

See Also:


Attachments
Patch (55.04 KB, patch)
2016-02-12 11:54 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (54.85 KB, patch)
2016-02-14 23:38 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (59.91 KB, patch)
2016-02-18 11:35 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (59.95 KB, patch)
2016-02-18 13:00 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (70.34 KB, patch)
2016-02-19 15:48 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (834.98 KB, application/zip)
2016-02-19 16:47 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (811.15 KB, application/zip)
2016-02-19 16:53 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (828.69 KB, application/zip)
2016-02-19 16:57 PST, Build Bot
no flags Details
Benchmark test result (65.02 KB, text/plain)
2016-02-19 23:09 PST, GSkachkov
no flags Details
Patch (70.35 KB, patch)
2016-02-20 00:37 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (70.34 KB, patch)
2016-02-22 12:19 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (70.34 KB, patch)
2016-02-22 13:35 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (70.36 KB, patch)
2016-02-24 02:19 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (65.89 KB, patch)
2016-02-26 07:41 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (69.91 KB, patch)
2016-02-27 15:02 PST, GSkachkov
beidson: review-
gskachkov: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (822.57 KB, application/zip)
2016-02-27 15:59 PST, Build Bot
no flags Details
Patch (72.27 KB, patch)
2016-02-28 09:28 PST, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2016-02-08 03:25:56 PST
In current implementation of arrow function we emit loading/putting this/super alway nevertheless if this/super are used inside of the arrow function.
Comment 1 GSkachkov 2016-02-12 11:54:56 PST
Created attachment 271198 [details]
Patch

Patch
Comment 2 GSkachkov 2016-02-14 23:38:27 PST
Created attachment 271327 [details]
Patch

Tiny fixes
Comment 3 Saam Barati 2016-02-16 14:17:57 PST
Comment on attachment 271327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271327&action=review

LGTM, just a few design suggestions and renaming suggestions.
Have you run perf numbers?

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:132
> +    bool isInnerArrowFunctionUseArguments() { return m_arrowFunctionCodeFeatures & ArgumentsArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseSuperCall() { return m_arrowFunctionCodeFeatures & SuperCallArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseSuperProperty() { return m_arrowFunctionCodeFeatures & SuperPropertyArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseEval() { return m_arrowFunctionCodeFeatures & EvalArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseThis() { return m_arrowFunctionCodeFeatures & ThisArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseNewTarget() { return m_arrowFunctionCodeFeatures & NewTargetArrowFunctionFeature; }

I would name these like "doAnyInnerArrowFunctionsX"
i.e:
isInnerArrowFunctionUseArguments => doAnyInnerArrowFunctionsUseArguments

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4051
> +bool BytecodeGenerator::isNewTargetUsedInArrowFunction()
> +{
> +    return m_codeBlock->isInnerArrowFunctionUseNewTarget() || m_codeBlock->isInnerArrowFunctionUseSuperCall();
> +}

Is "new.target" not allowed in eval?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:648
> +        void emitPutThisToArrowFunctionContextScope(bool = false);

Style: give this bool a name, like "shouldAlwaysPut" or something.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:724
> +        bool isThisUsedInArrowFunction();
> +        bool isNewTargetUsedInArrowFunction();
> +        bool isSuperUsedInArrowFunction();
> +        bool isArgumentsUsedInArrowFunction();

nit:  I would name these with "Inner" before "In"
like:
"isThisUsedInInnerArrowFunction"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764
> +            generator.emitPutThisToArrowFunctionContextScope(true);

Style: We usually write code like this by assigning the boolean as a local variable and passing the local variable to the function:
```
bool forcePutToScope = true;
generator.emitPutThisToArrowFunctionContextScope(forcePutToScope);
```

> Source/JavaScriptCore/parser/Parser.cpp:2067
> +            if (functionBodyType != StandardFunctionBodyBlock)
> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseSuperCall();

An alternative to repeatedly doing a scope search here is to just propagate this information
when we pop the scope. That's usually how we propagate information upwards. Look at popScopeInternal.
Also, instead of "functionBodyType != StandardFunctionBodyBlock", I think the code is easier to read
with affirmative conditions, like "bodyType == ArrowExpr || bodyType == ArrowBlock"

> Source/JavaScriptCore/parser/Parser.cpp:3644
> +            if (m_vm->propertyNames->eval == *ident)
> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseEval();
> +            else if (m_vm->propertyNames->arguments == *ident)
> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseArgumetns();

If you go with the popScopeInternalApproach, this can be determined then as well when we're already iterating over all used variables.

> Source/JavaScriptCore/parser/ParserModes.h:164
> +typedef uint16_t ArrowFunctionCodeFeatures;

this can be uint8_t

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:47
> +    unsigned innerArrowFunctionFeatures;

Why not ArrowFunctionCodeFeatures instead of unsigned?

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:92
> +    unsigned innerArrowFunctionFeatures : 31;

ditto.
Comment 4 GSkachkov 2016-02-18 11:35:34 PST
Created attachment 271677 [details]
Patch

Fix comments
Comment 5 GSkachkov 2016-02-18 12:54:18 PST
Comment on attachment 271327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271327&action=review

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:132
>> +    bool isInnerArrowFunctionUseNewTarget() { return m_arrowFunctionCodeFeatures & NewTargetArrowFunctionFeature; }
> 
> I would name these like "doAnyInnerArrowFunctionsX"
> i.e:
> isInnerArrowFunctionUseArguments => doAnyInnerArrowFunctionsUseArguments

Renamed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4051
>> +}
> 
> Is "new.target" not allowed in eval?

Ups. I forget about eval. Fixed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:648
>> +        void emitPutThisToArrowFunctionContextScope(bool = false);
> 
> Style: give this bool a name, like "shouldAlwaysPut" or something.

added

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:724
>> +        bool isArgumentsUsedInArrowFunction();
> 
> nit:  I would name these with "Inner" before "In"
> like:
> "isThisUsedInInnerArrowFunction"

Done

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764
>> +            generator.emitPutThisToArrowFunctionContextScope(true);
> 
> Style: We usually write code like this by assigning the boolean as a local variable and passing the local variable to the function:
> ```
> bool forcePutToScope = true;
> generator.emitPutThisToArrowFunctionContextScope(forcePutToScope);
> ```

Done

>> Source/JavaScriptCore/parser/Parser.cpp:2067
>> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseSuperCall();
> 
> An alternative to repeatedly doing a scope search here is to just propagate this information
> when we pop the scope. That's usually how we propagate information upwards. Look at popScopeInternal.
> Also, instead of "functionBodyType != StandardFunctionBodyBlock", I think the code is easier to read
> with affirmative conditions, like "bodyType == ArrowExpr || bodyType == ArrowBlock"

Refactored to use popScope and fixed condition

>> Source/JavaScriptCore/parser/Parser.cpp:3644
>> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseArgumetns();
> 
> If you go with the popScopeInternalApproach, this can be determined then as well when we're already iterating over all used variables.

Done

>> Source/JavaScriptCore/parser/ParserModes.h:164
>> +typedef uint16_t ArrowFunctionCodeFeatures;
> 
> this can be uint8_t

Changed

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:47
>> +    unsigned innerArrowFunctionFeatures;
> 
> Why not ArrowFunctionCodeFeatures instead of unsigned?

Fixed

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:92
>> +    unsigned innerArrowFunctionFeatures : 31;
> 
> ditto.

Done
Comment 6 GSkachkov 2016-02-18 12:59:13 PST
(In reply to comment #3)
> Comment on attachment 271327 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271327&action=review
> 
> LGTM, just a few design suggestions and renaming suggestions.
> Have you run perf numbers?
> 

Not yet, I'm going to run it tomorrow. I think we need to add several more benchmark tests with eval and classes
Comment 7 GSkachkov 2016-02-18 13:00:03 PST
Created attachment 271685 [details]
Patch

Fix Changelog & remove commented out code
Comment 8 GSkachkov 2016-02-19 13:20:33 PST
Comment on attachment 271685 [details]
Patch

Found bugs in using arrow function inside of the constructor
Comment 9 GSkachkov 2016-02-19 15:48:18 PST
Created attachment 271811 [details]
Patch

Fix bugs: putting 'this' to arrow function context
Comment 10 Build Bot 2016-02-19 16:47:48 PST
Comment on attachment 271811 [details]
Patch

Attachment 271811 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/856427

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html
js/regress/arrowfunction-call-in-class-method.html
js/regress/arrowfunction-call-in-function.html
Comment 11 Build Bot 2016-02-19 16:47:51 PST
Created attachment 271820 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 12 Build Bot 2016-02-19 16:53:12 PST
Comment on attachment 271811 [details]
Patch

Attachment 271811 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/856428

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html
js/regress/arrowfunction-call-in-class-method.html
js/regress/arrowfunction-call-in-function.html
Comment 13 Build Bot 2016-02-19 16:53:15 PST
Created attachment 271821 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 14 Build Bot 2016-02-19 16:57:28 PST
Comment on attachment 271811 [details]
Patch

Attachment 271811 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/856430

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html
js/regress/arrowfunction-call-in-class-method.html
js/regress/arrowfunction-call-in-function.html
Comment 15 Build Bot 2016-02-19 16:57:31 PST
Created attachment 271823 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 16 GSkachkov 2016-02-19 23:09:46 PST
Created attachment 271852 [details]
Benchmark test result

See Benchmark test result in attach
Comment 17 GSkachkov 2016-02-19 23:31:26 PST
(In reply to comment #3)
> Comment on attachment 271327 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271327&action=review
> 
> LGTM, just a few design suggestions and renaming suggestions.
> Have you run perf numbers?
> 

I think it quite good figures for arrow function:
                                                      before_patch                 after_patch
   arrowfunction-call-in-class-constructor          300.1710+-13.5061    ^    191.4837+-4.5846        ^ definitely 1.5676x faster
   arrowfunction-call-in-class-method                91.2163+-7.7129     ^     29.3135+-10.2887       ^ definitely 3.1118x faster
   arrowfunction-call-in-function                    96.5411+-31.9533    ^     19.6813+-1.5141        ^ definitely 4.9052x faster
   arrowfunction-call                                33.9300+-3.3111     ^     27.0056+-0.9594        ^ definitely 1.2564x faster
Comment 18 GSkachkov 2016-02-20 00:37:26 PST
Created attachment 271854 [details]
Patch

Fix tests
Comment 19 Saam Barati 2016-02-21 15:18:56 PST
Comment on attachment 271854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271854&action=review

r=me w/ comments.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050
> +    return m_codeBlock->doAnyInnerArrowFunctionsUseNewTarget() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseEval();

I was wrong about this. new.target is prohibited inside eval. Sorry for stating the wrong thing in my previous review.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4055
> +    return m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperProperty() || m_codeBlock->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();

Also, 'super' isn't allowed inside an eval. It's a SyntaxError.

> Source/JavaScriptCore/parser/Parser.h:473
> +        if (m_isArrowFunction && m_usesEval)
> +            setInnerArrowFunctionUseEval();
> +        
> +        if (m_isArrowFunction && m_vm->propertyNames->arguments == *ident)
> +            setInnerArrowFunctionUseArgumetns();

An alternative to doing this at every useVariable(.) call
is to just do it once when popScope happens.

> Source/JavaScriptCore/parser/Parser.h:953
> +        // We do not up arrow function features for ordinary function

comment not needed.
Comment 20 Saam Barati 2016-02-21 15:24:49 PST
(In reply to comment #17)
> (In reply to comment #3)
> > Comment on attachment 271327 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=271327&action=review
> > 
> > LGTM, just a few design suggestions and renaming suggestions.
> > Have you run perf numbers?
> > 
> 
> I think it quite good figures for arrow function:
>                                                       before_patch          
> after_patch
>    arrowfunction-call-in-class-constructor          300.1710+-13.5061    ^  
> 191.4837+-4.5846        ^ definitely 1.5676x faster
>    arrowfunction-call-in-class-method                91.2163+-7.7129     ^  
> 29.3135+-10.2887       ^ definitely 3.1118x faster
>    arrowfunction-call-in-function                    96.5411+-31.9533    ^  
> 19.6813+-1.5141        ^ definitely 4.9052x faster
>    arrowfunction-call                                33.9300+-3.3111     ^  
> 27.0056+-0.9594        ^ definitely 1.2564x faster

Nice.

It would be worthwhile to open a bug to rewrite the deltablue benchmark
(or some other benchmark that tests function calling) to use
arrow functions everywhere. That way we can compare that performance
to the performance of just using ordinary functions. And we can track
performance in arrow functions w/ that benchmark.
Comment 21 GSkachkov 2016-02-22 12:19:17 PST
Created attachment 271939 [details]
Patch

Fix last comments
Comment 22 GSkachkov 2016-02-22 12:53:51 PST
Comment on attachment 271854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271854&action=review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050
>> +    return m_codeBlock->doAnyInnerArrowFunctionsUseNewTarget() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseEval();
> 
> I was wrong about this. new.target is prohibited inside eval. Sorry for stating the wrong thing in my previous review.

Fixed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4055
>> +    return m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperProperty() || m_codeBlock->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();
> 
> Also, 'super' isn't allowed inside an eval. It's a SyntaxError.

I fixed. However we have issue where will fix this SyntaxError :-)

>> Source/JavaScriptCore/parser/Parser.h:473
>> +            setInnerArrowFunctionUseArgumetns();
> 
> An alternative to doing this at every useVariable(.) call
> is to just do it once when popScope happens.

OK. Refactored.

>> Source/JavaScriptCore/parser/Parser.h:953
>> +        // We do not up arrow function features for ordinary function
> 
> comment not needed.

Removed
Comment 23 GSkachkov 2016-02-22 12:56:21 PST
(In reply to comment #20)
> (In reply to comment #17)
> > (In reply to comment #3)
> > > Comment on attachment 271327 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=271327&action=review
> > > 
> > > LGTM, just a few design suggestions and renaming suggestions.
> > > Have you run perf numbers?
> > > 
> > 
> > I think it quite good figures for arrow function:
> >                                                       before_patch          
> > after_patch
> >    arrowfunction-call-in-class-constructor          300.1710+-13.5061    ^  
> > 191.4837+-4.5846        ^ definitely 1.5676x faster
> >    arrowfunction-call-in-class-method                91.2163+-7.7129     ^  
> > 29.3135+-10.2887       ^ definitely 3.1118x faster
> >    arrowfunction-call-in-function                    96.5411+-31.9533    ^  
> > 19.6813+-1.5141        ^ definitely 4.9052x faster
> >    arrowfunction-call                                33.9300+-3.3111     ^  
> > 27.0056+-0.9594        ^ definitely 1.2564x faster
> 
> Nice.
> 
> It would be worthwhile to open a bug to rewrite the deltablue benchmark
> (or some other benchmark that tests function calling) to use
> arrow functions everywhere. That way we can compare that performance
> to the performance of just using ordinary functions. And we can track
> performance in arrow functions w/ that benchmark.

I"ve added your comment to the issue that related to arrow function benchmark tests
https://bugs.webkit.org/show_bug.cgi?id=148034
Comment 24 Saam Barati 2016-02-22 13:20:32 PST
Comment on attachment 271939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271939&action=review

> Source/JavaScriptCore/parser/Parser.h:493
> +    void setInnerArrowFunctionUseArgumetns() { m_innerArrowFunctionFeatures |= ArgumentsArrowFunctionFeature; }

I missed this earlier, but the spelling is wrong here.
Comment 25 GSkachkov 2016-02-22 13:35:34 PST
Created attachment 271952 [details]
Patch

Fix TYPO
Comment 26 GSkachkov 2016-02-22 13:37:17 PST
Comment on attachment 271939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271939&action=review

>> Source/JavaScriptCore/parser/Parser.h:493
>> +    void setInnerArrowFunctionUseArgumetns() { m_innerArrowFunctionFeatures |= ArgumentsArrowFunctionFeature; }
> 
> I missed this earlier, but the spelling is wrong here.

Oh, sorry. Fixed.
Comment 27 Saam Barati 2016-02-23 23:56:28 PST
Comment on attachment 271952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271952&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4010
> +RegisterID* BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment(Identifier identifier)

This type should be "const Identfifier&"
Comment 28 GSkachkov 2016-02-24 02:19:07 PST
Created attachment 272107 [details]
Patch

Patch comming
Comment 29 GSkachkov 2016-02-24 03:44:53 PST
(In reply to comment #27)
> Comment on attachment 271952 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271952&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4010
> > +RegisterID* BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment(Identifier identifier)
> 
> This type should be "const Identfifier&"

Fixed
Comment 30 WebKit Commit Bot 2016-02-24 09:36:08 PST
Comment on attachment 272107 [details]
Patch

Clearing flags on attachment: 272107

Committed r197033: <http://trac.webkit.org/changeset/197033>
Comment 31 WebKit Commit Bot 2016-02-24 09:36:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 Ryan Haddad 2016-02-24 10:53:22 PST
This change may have caused 14 new JSC test failures

** The following JSC stress test failures have been introduced:
	stress/proxy-call.js.always-trigger-copy-phase
	stress/proxy-call.js.default
	stress/proxy-call.js.default-ftl
	stress/proxy-call.js.dfg-eager
	stress/proxy-call.js.dfg-eager-no-cjit-validate
	stress/proxy-call.js.dfg-maximal-flush-validate-no-cjit
	stress/proxy-call.js.ftl-eager
	stress/proxy-call.js.ftl-eager-no-cjit
	stress/proxy-call.js.ftl-no-cjit-no-inline-validate
	stress/proxy-call.js.ftl-no-cjit-no-put-stack-validate
	stress/proxy-call.js.ftl-no-cjit-small-pool
	stress/proxy-call.js.ftl-no-cjit-validate-sampling-profiler
	stress/proxy-call.js.no-cjit-validate-phases
	stress/proxy-call.js.no-llint

<https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/3604/steps/jscore-test/logs/stdio>
Comment 33 Saam Barati 2016-02-24 11:06:09 PST
(In reply to comment #32)
> This change may have caused 14 new JSC test failures
> 
> ** The following JSC stress test failures have been introduced:
> 	stress/proxy-call.js.always-trigger-copy-phase
> 	stress/proxy-call.js.default
> 	stress/proxy-call.js.default-ftl
> 	stress/proxy-call.js.dfg-eager
> 	stress/proxy-call.js.dfg-eager-no-cjit-validate
> 	stress/proxy-call.js.dfg-maximal-flush-validate-no-cjit
> 	stress/proxy-call.js.ftl-eager
> 	stress/proxy-call.js.ftl-eager-no-cjit
> 	stress/proxy-call.js.ftl-no-cjit-no-inline-validate
> 	stress/proxy-call.js.ftl-no-cjit-no-put-stack-validate
> 	stress/proxy-call.js.ftl-no-cjit-small-pool
> 	stress/proxy-call.js.ftl-no-cjit-validate-sampling-profiler
> 	stress/proxy-call.js.no-cjit-validate-phases
> 	stress/proxy-call.js.no-llint
> 
> <https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/3604/steps/jscore-
> test/logs/stdio>

It did. I'm looking into a fix now.
Comment 34 WebKit Commit Bot 2016-02-24 12:40:01 PST
Re-opened since this is blocked by bug 154649
Comment 35 GSkachkov 2016-02-26 07:41:32 PST
Created attachment 272326 [details]
Patch

Fixed version
Comment 36 Saam Barati 2016-02-26 12:17:43 PST
Comment on attachment 272326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272326&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575
> +    if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isThisUsedInInnerArrowFunction()))

Is this change here correct?
Does it work for:
class A { getValue() { ... } }
class B extends A { constructor() { arr = ()=>super.getValue(); arr(); super(); }}
Comment 37 GSkachkov 2016-02-26 13:07:43 PST
It should work, we emit TDZ always when access to the super. 
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L922
Comment 38 GSkachkov 2016-02-26 13:08:34 PST
(In reply to comment #36)
> Comment on attachment 272326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272326&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575
> > +    if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isThisUsedInInnerArrowFunction()))
> 
> Is this change here correct?
> Does it work for:
> class A { getValue() { ... } }
> class B extends A { constructor() { arr = ()=>super.getValue(); arr();
> super(); }}

It should work, we emit TDZ always when access to the super. 
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L922
Comment 39 GSkachkov 2016-02-26 13:43:09 PST
Comment on attachment 272326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272326&action=review

> Source/JavaScriptCore/parser/Parser.cpp:3890
> +        if (currentFunctionScope()->isArrowFunction()) {
> +            if (currentFunctionScope()->hasDirectSuper())
> +                currentFunctionScope()->setInnerArrowFunctionUseSuperCall();
> +        
> +            if (currentFunctionScope()->needsSuperBinding())
> +                currentFunctionScope()->setInnerArrowFunctionUseSuperProperty();
> +        }
> +

Main change to fix error in tests
Comment 40 GSkachkov 2016-02-27 15:02:05 PST
Created attachment 272424 [details]
Patch

Fix comments & Stress tests are executed
Comment 41 Build Bot 2016-02-27 15:59:13 PST
Comment on attachment 272424 [details]
Patch

Attachment 272424 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/892438

New failing tests:
storage/indexeddb/key-generator.html
Comment 42 Build Bot 2016-02-27 15:59:17 PST
Created attachment 272425 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 43 Saam Barati 2016-02-27 16:19:58 PST
Comment on attachment 272424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review

> Source/JavaScriptCore/parser/Parser.cpp:3883
> +        if (currentFunctionScope()->isArrowFunction()) {

I don't understand this code. Why does this happen unconditionally?
Shouldn't it depend on if "baseIsSuper == true"?
Also, does this miss the case where you just have "super" without
a property access or function call afterward?
I.e:
()=>super;
Comment 44 Saam Barati 2016-02-27 16:20:00 PST
Comment on attachment 272424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review

> Source/JavaScriptCore/parser/Parser.cpp:3883
> +        if (currentFunctionScope()->isArrowFunction()) {

I don't understand this code. Why does this happen unconditionally?
Shouldn't it depend on if "baseIsSuper == true"?
Also, does this miss the case where you just have "super" without
a property access or function call afterward?
I.e:
()=>super;
Comment 45 Saam Barati 2016-02-27 16:20:01 PST
Comment on attachment 272424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review

> Source/JavaScriptCore/parser/Parser.cpp:3883
> +        if (currentFunctionScope()->isArrowFunction()) {

I don't understand this code. Why does this happen unconditionally?
Shouldn't it depend on if "baseIsSuper == true"?
Also, does this miss the case where you just have "super" without
a property access or function call afterward?
I.e:
()=>super;
Comment 46 Saam Barati 2016-02-27 16:20:05 PST
Comment on attachment 272424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review

> Source/JavaScriptCore/parser/Parser.cpp:3883
> +        if (currentFunctionScope()->isArrowFunction()) {

I don't understand this code. Why does this happen unconditionally?
Shouldn't it depend on if "baseIsSuper == true"?
Also, does this miss the case where you just have "super" without
a property access or function call afterward?
I.e:
()=>super;
Comment 47 Saam Barati 2016-02-27 16:21:16 PST
(Ah, sorry for the repeated same comment. Bugzilla doesn't like me today.)
Comment 48 GSkachkov 2016-02-28 09:28:25 PST
Created attachment 272456 [details]
Patch

Update patch
Comment 49 GSkachkov 2016-02-28 09:34:13 PST
Comment on attachment 272424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272424&action=review

>>>>> Source/JavaScriptCore/parser/Parser.cpp:3883
>>>>> +        if (currentFunctionScope()->isArrowFunction()) {
>>>> 
>>>> I don't understand this code. Why does this happen unconditionally?
>>>> Shouldn't it depend on if "baseIsSuper == true"?
>>>> Also, does this miss the case where you just have "super" without
>>>> a property access or function call afterward?
>>>> I.e:
>>>> ()=>super;
>>> 
>>> I don't understand this code. Why does this happen unconditionally?
>>> Shouldn't it depend on if "baseIsSuper == true"?
>>> Also, does this miss the case where you just have "super" without
>>> a property access or function call afterward?
>>> I.e:
>>> ()=>super;
>> 
>> I don't understand this code. Why does this happen unconditionally?
>> Shouldn't it depend on if "baseIsSuper == true"?
>> Also, does this miss the case where you just have "super" without
>> a property access or function call afterward?
>> I.e:
>> ()=>super;
> 
> I don't understand this code. Why does this happen unconditionally?
> Shouldn't it depend on if "baseIsSuper == true"?
> Also, does this miss the case where you just have "super" without
> a property access or function call afterward?
> I.e:
> ()=>super;

My idea was to check hasDirectSuper & needsSuperBinding that is set when baseIsSuper == true by (setNeedsSuperBinding & setHasDirectSuper), but I see you point, that my current solution is not optimal, it is my fault. I'll rewrite. 

In general '()=>super;' should return syntax error: 'Cannot reference super', and it do.
Comment 50 Saam Barati 2016-02-28 10:09:05 PST
Comment on attachment 272456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272456&action=review

> Source/JavaScriptCore/parser/Parser.h:488
> +    void setInnerArrowFunctionUseSuperCall() { m_innerArrowFunctionFeatures |= SuperCallInnerArrowFunctionFeature; }

Nit: all of these setInner* should replace "use" with "uses"
Comment 51 GSkachkov 2016-02-28 21:00:08 PST
Committed r 197296: <http://trac.webkit.org/changeset/197296>
Comment 52 GSkachkov 2016-02-29 00:04:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 53 GSkachkov 2016-06-06 23:23:20 PDT
*** Bug 149933 has been marked as a duplicate of this bug. ***
Comment 54 Brady Eidson 2017-08-19 16:00:39 PDT
Comment on attachment 272424 [details]
Patch

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.
Comment 55 GSkachkov 2017-08-20 00:51:23 PDT
(In reply to Brady Eidson from comment #54)
> Comment on attachment 272424 [details]
> Patch
> 
> r-, as this has been pending review for over a year now. It is
> near-impossible that this patch still applies to trunk and unlikely to still
> be relevant in its current form.

Ohh it is already landed patch. Will clear all flags