1410640 - Allow nursery allocation for arrow functions?

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P2)

Description

[André Bargull [:anba]]

bug 989414 only enabled nursery allocation for normal functions, so currently arrow functions are still always allocated in the tenured heap. Is there any technical limitation which prohibits nursery allocation of arrow functions? And if there's no technical limitation, does it make sense to allow nursery allocation of arrow functions resp. is more useful not to allow nursery allocation of arrow functions?

Comment 1

[Jon Coppeard (:jonco)]

I haven't looked but don't see why there should be any reason not to do this.  Thanks for pointing this out.

Comment 2

[Paul Bone [:pbone]]

Does this refer to the function closure object itself or allocations that the arrow function performs?

Comment 3

[André Bargull [:anba]]

To the arrow function itself. Specifically I was wondering if we can align the CloneFunctionObjectIfNotSingleton calls in js::Lambda [1] and js::LambdaArrow [2].

[1] https://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/js/src/vm/Interpreter.cpp#4401
[2] https://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/js/src/vm/Interpreter.cpp#4414-4415

Comment 4

[Jan de Mooij [:jandem]]

Good find, we should fix this. André can you take this? Else I can do it.

Comment 5

[André Bargull [:anba]]

Attachment: bug1410640.patch

https://hg.mozilla.org/mozilla-central/rev/8c234572141a2593807d8ff5960f6c7789305da7 only changed js::Lambda(...) to enable nursery allocation of normal functions, so I assume for arrow functions we also just need to adjust js::LambdaArrow(...)?


The patch improves this µ-benchmark from 360ms to 20ms for me:

    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        q += (() => i)();
    }
    print(dateNow() - t, q);

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8921411 [details] [diff] [review]
bug1410640.patch

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

Great, thanks for fixing this.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #5)
> The patch improves this µ-benchmark from 360ms to 20ms for me:
> 
>     var q = 0;
>     var t = dateNow();
>     for (var i = 0; i < 1000000; ++i) {
>         q += (() => i)();
>     }
>     print(dateNow() - t, q);

Is this with Ion disabled? Ion should do nursery allocation inline in CodeGenerator::visitLambdaArrow so I wonder if that's not working...

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Is this with Ion disabled? Ion should do nursery allocation inline in
> CodeGenerator::visitLambdaArrow so I wonder if that's not working...

Oh maybe we fill up the nursery and then keep calling LambdaArrow OOL without collecting the nursery?

Comment 9

[André Bargull [:anba]]

The actual test case I used was:
```
function f() {
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        q += (() => i)();
    }
    return [dateNow() - t, q];
}
for (var i = 0; i < 10; ++i) print(f());
```

Running only the function content of |f| in global scope is already fast without this patch:
```
var q = 0;
var t = dateNow();
for (var i = 0; i < 1000000; ++i) {
    q += (() => i)();
}
print(dateNow() - t, q);
```

So there seems to be a difference whether or not the benchmark code is run within another function?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #9)
> So there seems to be a difference whether or not the benchmark code is run
> within another function?

When we run it in the global scope, |i| is a global name and we can optimize away the arrow allocation. When the code is in a function, we have to get |i| from the arrow's environment chain and we don't optimize away the arrow allocation.

Comment 11

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67c9b7e427125fafea5b1cfda4babd45760fd039

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f17f122eb7c
Enable nursery allocation of arrow functions. r=jonco

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1f17f122eb7c