Closed Bug 989204 Opened 10 years ago Closed 10 years ago

Don't use bound functions for arrow functions


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jandem, Assigned: jandem)




(2 files, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
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;
print(new Date - t);
Attached patch Part 1 - TestsSplinter Review
Some tests I wrote for issues I ran into, also a test for bug 889158.
Attachment #8398502 - Flags: review?(jorendorff)
Requesting review from bhackett for the JIT changes, jorendorff for everything else.

Passes --tbpl. Pushed to Try:

Fortunately the browser uses a ton of arrow functions; let's see how this turns out.
Attachment #8398337 - Attachment is obsolete: true
Attachment #8398541 - Flags: review?(jorendorff)
Attachment #8398541 - Flags: review?(bhackett1024)
Attachment #8398541 - Flags: review?(bhackett1024) → review+
The browser was crashing on startup due to some XDR/cloning issue. Everything looks green now (so far):

Only need review for the non-JIT changes.
Attachment #8398541 - Attachment is obsolete: true
Attachment #8398541 - Flags: review?(jorendorff)
Attachment #8398689 - Flags: review?(jorendorff)
Attachment #8398502 - Flags: review?(jorendorff) → review+
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.
Attachment #8398689 - Flags: review?(jorendorff) → review+
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.