1175466 - Allocate arguments objects in the nursery

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Most benchmarks don't create (non-lazy) arguments objects, but some websites and frameworks use them a lot. We can do much better than we're doing now.

This patch lets us nursery allocate ArgumentsObject and its ArgumentsData. It's nice because (1) arguments objects are usually short-lived, so they really benefit from GGC and (2) we avoid a (slow) malloc for the ArgumentsData.

For the micro-benchmark below I get (Mac 32-bit):

before: 307 ms
after:  172 ms

The ArgumentsObject allocation path is still pretty slow but this is a good start.

var args;
function g() {
    args = arguments;
}
function f() {
    for (var i=0; i<1000000; i++)
	g(i);
}
var t = new Date;
f();
print(new Date - t);

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8623590 [details] [diff] [review]
Patch

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

Nice!

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67296c6ede81

Comment 3

[Jan de Mooij [:jandem]]

Bleh, it was green this morning but some GC changes broke it in the meantime:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f942c1cd0d0

Comment 4

[Jan de Mooij [:jandem]]

So with this patch + bug 1175642 we assert in assertHasValueEdge with the following testcase:

function f() {
    arguments[0] = {};
    arguments[0] = {};
}
f(1);

If the argument's HeapValue is in the nursery, ValueEdge::maybeInRememberedSet will return false and we don't add any ValueEdge to the StoreBuffer. Then later on assertHasValueEdge fails.

Changing assertHasValueEdge to this fixes it:

    void assertHasValueEdge(Value* vp) const {
        MOZ_ASSERT_IF(ValueEdge(vp).maybeInRememberedSet(nursery_), bufferVal.has(ValueEdge(vp)));
    }

Terrence does this make sense? Is there a better way to fix this?

Comment 5

[Terrence Cole [:terrence]]

Yes, I think the assertion just does not consider nursery->nursery edges. Please fix it inline, or I'll whip up a separate patch today.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Fix asserts

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8625892 [details] [diff] [review]
Fix asserts

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

Sorry for the bustage!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26275d8c0c95

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26275d8c0c95