André Bargull [:anba] Assignee
	Description • 7 years ago

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?

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 1 • 7 years ago

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

	Paul Bone [:pbone]
	Comment 2 • 7 years ago

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

	André Bargull [:anba] Assignee
	Comment 3 • 7 years ago

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

	Jan de Mooij [:jandem]
	Comment 4 • 7 years ago

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

	André Bargull [:anba] Assignee
	Comment 5 • 7 years ago

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);

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 6 • 7 years ago

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

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

Great, thanks for fixing this.

	Jan de Mooij [:jandem]
	Comment 7 • 7 years ago

(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...

	Jan de Mooij [:jandem]
	Comment 8 • 7 years ago

(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?

	André Bargull [:anba] Assignee
	Comment 9 • 7 years ago

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?

	Jan de Mooij [:jandem]
	Comment 10 • 7 years ago

(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.

	André Bargull [:anba] Assignee
	Comment 11 • 7 years ago

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

	Pulsebot
	Comment 12 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f17f122eb7c
Enable nursery allocation of arrow functions. r=jonco

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 13 • 7 years ago
bugherder

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