Bug 168829 – Use of arguments in arrow function is slow

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

[Saam Barati]

fn = function() {
    var args = arguments;
    return () => args[0];
  }(1)

vs.

fn = function() {
    return () => arguments[0];
  }(1)

The first runs much faster than the second

[GSkachkov]

I think this issue is related to mistake in calculation of the variable needsArguments in BytecodeGenerator.cpp. Using arguments in arrow function leads to emitting create_lexical_environment and create_direct_arguments twice, at first for wrapper function second for arrow function. 
I'll try to prepare fix during weekend

[Saam Barati]

(In reply to comment #1)
> I think this issue is related to mistake in calculation of the variable
> needsArguments in BytecodeGenerator.cpp. Using arguments in arrow function
> leads to emitting create_lexical_environment and create_direct_arguments
> twice, at first for wrapper function second for arrow function. 
> I'll try to prepare fix during weekend

Sounds good.

[GSkachkov]

I fixed issue with needsArguments and byte code looks the same except difference in type of variable and still difference in performance. Difference is type of variable during resolve scope in first example is ClosureVar and in second case is Dynamic. See bytecode for second cas:

[   0] enter             
[   1] get_scope         loc3
[   3] mov               loc4, loc3
[   6] create_lexical_environment loc5, loc3, Cell: 0x1081871a0 (0x1081e8a20:[SymbolTable, {}, NonArray, Leaf]), ID: 20(const0), Undefined(const1)
[  11] mov               loc3, loc5
[  14] create_direct_arguments loc6
[  16] put_to_scope      loc5, arguments(@id0), loc6, 2052<ThrowIfNotFound|LocalClosureVar|NotInitialization>, <structure>, 0
[  23] new_func_exp      loc8, loc3, f0
[  27] ret               loc8

Identifiers:
  id0 = arguments

Constants:
   k0 = Cell: 0x1081871a0 (0x1081e8a20:[SymbolTable, {}, NonArray, Leaf]), ID: 20
   k1 = Undefined

#A6UXLB:[0x108168520->0x1081b7020, NoneFunctionCall, 31 (NeverInline)]: 31 m_instructions; 248 bytes; 1 parameter(s); 10 callee register(s); 6 variable(s); scope at loc3
[   0] enter             
[   1] get_scope         loc3
[   3] mov               loc4, loc3
[   6] resolve_scope     loc7, loc3, arguments(@id0), <DYNAMIC>, 0, 0x0
[  13] get_from_scope    loc8, loc7, arguments(@id0), 2060<ThrowIfNotFound| DYNAMIC |NotInitialization>, 0    predicting None
[  21] get_by_val        loc6, loc8, Int32: 0(const0)    Original; predicting None
[  27] ret               loc6
[  29] ret               Undefined(const1) 

Is it possible fix this?

[GSkachkov]

Created attachment 302789 [details]
Patch

Patch for review

[Saam Barati]

Comment on attachment 302789 [details]
Patch

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

r=me.

> Source/JavaScriptCore/ChangeLog:12
> +        patch it can be ClosureVar, that increase performance of access to arguments variable
> +        inside of the arrow function. 

How much faster is it now? Can you write that here.

> Source/JavaScriptCore/runtime/JSScope.cpp:-55
> -        if (ident == exec->propertyNames().arguments) {

Nice. This looks like old code to me that should no longer be needed.

> JSTests/stress/arrowfunction-lexical-bind-arguments-non-strict-1.js:269
> +var fn = function() {
> +    return () => arguments[0];
> +}(1);
> +
> +for (var i = 0; i < 10000; i++) {
> +    testCase(fn(2), 1); 
> +}
> +
> +var fn = function() {
> +    var args = arguments;
> +    return () => args[0];
> +}(1);
> +
> +for (var i = 0; i < 10000; i++) {
> +    testCase(fn(2), 1);
> +}

Can you add these tests under the microbenchmarks directory?

[Saam Barati]

Can you also run the layout tests locally, or wait for EWS to come back online before landing?

[GSkachkov]

Created attachment 302959 [details]
Patch

Fix comments

[WebKit Commit Bot]

Comment on attachment 302959 [details]
Patch

Clearing flags on attachment: 302959

Committed r213165: <http://trac.webkit.org/changeset/213165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.