Don't use bound functions for arrow functions

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

5 years ago
Posted 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;
f();
print(new Date - t);
Assignee

Comment 1

5 years ago
Some tests I wrote for issues I ran into, also a test for bug 889158.
Attachment #8398502 - Flags: review?(jorendorff)
Assignee

Comment 2

5 years ago
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.
Attachment #8398337 - Attachment is obsolete: true
Attachment #8398541 - Flags: review?(jorendorff)
Attachment #8398541 - Flags: review?(bhackett1024)
Attachment #8398541 - Flags: review?(bhackett1024) → review+
Assignee

Comment 3

5 years ago
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.
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+
https://hg.mozilla.org/mozilla-central/rev/8cef7a6e9f79
https://hg.mozilla.org/mozilla-central/rev/9517fe15e2c2
https://hg.mozilla.org/mozilla-central/rev/7cdf043c7666
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee

Updated

4 years ago
Duplicate of this bug: 921813
You need to log in before you can comment on or make changes to this bug.