989204 - Don't use bound functions for arrow functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Arrow functions are currently implemented using a bound function wrapper that passes the modified |this|. This is not ideal for a number of reasons:

(1) It's hard to optimize arrow function calls as they go through the VM fun.bind machinery.
(2) We have to allocate a second JSFunction when cloning arrow lambdas.
(3) It's not spec-compliant, at least in its current form: the bound function is observable via arguments.callee, see bug 889158.

I'm attaching a patch that removes the function binding and instead stores the lexical |this| Value for arrow functions in an extended slot. JSOP_THIS/ComputeThis now use this (no pun intended) Value for arrow functions.

This makes arrow function calls as fast as "normal" function calls, and Ion is now able to inline them. For instance, for the micro-benchmark below I get:

before: 1096 ms
after :    3 ms

var sum = (x, y) => x + y;
function f() {
    var res = 0;
    for (var i=0; i<10000000; i++)
	res = sum(i, 1);
    return res;
}
var t = new Date;
f();
print(new Date - t);

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Tests

Some tests I wrote for issues I ran into, also a test for bug 889158.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Make arrow functions fast

Requesting review from bhackett for the JIT changes, jorendorff for everything else.

Passes jit_test.py --tbpl. Pushed to Try:

https://tbpl.mozilla.org/?tree=Try&rev=acbba33d927c

Fortunately the browser uses a ton of arrow functions; let's see how this turns out.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Make arrow functions fast

The browser was crashing on startup due to some XDR/cloning issue. Everything looks green now (so far):

https://tbpl.mozilla.org/?tree=Try&rev=7d2cfcf45e9c

Only need review for the non-JIT changes.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8398689 [details] [diff] [review]
Part 2 - Make arrow functions fast

Review of attachment 8398689 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfun.cpp
@@ +519,5 @@
> +        /*
> +         * Arrow functions use the first extended slot to store their lexical
> +         * |this|.
> +         */
> +        clone->setExtendedSlot(0, srcFun->getExtendedSlot(0));

The caller is responsible for populating this slot, right? If so, populating it with a different value here seems like a bit of a red herring. Delete this block?

@@ -1243,5 @@
> -    if (constructing && fun->isArrow()) {
> -        /*
> -         * Per spec, arrow functions do not even have a [[Construct]] method.
> -         * So before anything else, if we are an arrow function, make sure we
> -         * don't even get here. You never saw me. Burn this comment.

thank you

::: js/src/jsfun.h
@@ +539,5 @@
>  
> +    static inline size_t offsetOfExtendedSlot(size_t which) {
> +        MOZ_ASSERT(which < NUM_EXTENDED_SLOTS);
> +        return offsetof(FunctionExtended, extendedSlots) + which * sizeof(extendedSlots[0]);
> +    }

Can we name this offsetOfArrowThisSlot()? Otherwise each caller has a magic constant 0 in it.

Comment 5

[Jan de Mooij [:jandem]]

Pushed with comments addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cef7a6e9f79
https://hg.mozilla.org/integration/mozilla-inbound/rev/9517fe15e2c2

(Comment 3 has a green Try run.)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8cef7a6e9f79
https://hg.mozilla.org/mozilla-central/rev/9517fe15e2c2
https://hg.mozilla.org/mozilla-central/rev/7cdf043c7666

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44c63bf18e74