1772597 - Tenuring tracer computes wrong size when tenuring Tuples

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Tim Chevalier]

+++ This bug was initially created as a clone of Bug #1763874 +++

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.93 Safari/537.36

Steps to reproduce:

Run the following test case (possibly not minimal):

gczeal(14);

var c = ["a", "b"];
var t = Tuple.from(c);

for (i = 0; i < 3; i++) {
    c = ["a", "b"];
    t = Tuple.from(c);
    gc();
}

Actual results:

mach jit-test # test file...
[...]
Assertion failure: result.tenuredBytes <= previousGC.nurseryUsedBytes, at /home/tjc/gecko-fork/js/src/gc/Nursery.cpp:1115

Expected results:

The test should pass. I'll have a patch for this ready next week.

The GC has an implicit assumption that only arrays can have fixed elements. Tuples also have fixed elements, but were being treated as if they don't because the code checks whether an object's class is ArrayObject or TypedArrayProject as a proxy for checking if it has fixed elements. This is related to Bug 1763874 -- but that wasn't a complete fix, because the code that promotes objects to the tenured heap still had the wrong assumption baked in.

So why didn't the test for Bug 1763874 expose this bug? Inexplicably, this assertion failure didn't appear until I integrated the following patch into my working branch: https://hg.mozilla.org/mozilla-unified/rev/516249a8f5c5 . This patch adds a new global function to TestingFunctions.cpp. That was how I found the bug, but through experimentation I determined that commenting out any function in TestingFunctions.cpp would mask the bug; it could also be triggered by adding several global functions to the test case itself. My guess is that when the global object has more than a certain number of properties (I noticed that the bug is triggered when the global object has exactly 511 dynamic slots, and isn't triggered when it has fewer than 511 dynamic slots), allocation behavior changes somehow and the tenured / nursery sizes change so that the assertion starts failing.

I think that suggests that the assertion (this one: https://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#1115 ) needs to be made stricter and/or there's another bug, but this is all I know for now.

Comment 1

[Tim Chevalier]

Attachment: Bug 1772597: Use tighter upper bound for tenured bytes in GC assertion r=sfink

The calculation for the previous GC's used nursery space should
exclude the space used for chunk headers, since this space is not
available for heap data and thus can't be promoted to the tenured heap.

Making this assertion stricter doesn't cause any pre-existing tests to fail,
but is necessary to minimize the test case for this bug.

Comment 2

[Tim Chevalier]

Attachment: Bug 1772597: Fix TenuringTracer::moveToTenuredSlow() to update tenuredSize correctly when tenuring a Tuple r=sfink

The case in moveToTenuredSlow() that previously only applied to arrays
should also apply to tuples. Because this case wasn't handling tuples,
tuple elements were getting double-counted (first when computing the
destination size based on the destination alloc kind; then when adding
the return value of moveElementsToTenured()). This in turn caused
the value reported by the GC for the number of tenured bytes to be
inaccurately high, causing an assertion failure in Nursery::collect()
because the reported amount of tenured data is greater than the size of
the nursery from the previous collection.

Depends on D148426

Comment 3

[Tim Chevalier]

Ready to land. Green try build at https://treeherder.mozilla.org/jobs?repo=try&revision=4a85717998a55cd3a6e18469c72bb1ae85be24c8

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/634a46da0bdc
Use tighter upper bound for tenured bytes in GC assertion r=sfink
https://hg.mozilla.org/integration/autoland/rev/577530c09b82
Fix TenuringTracer::moveToTenuredSlow() to update tenuredSize correctly when tenuring a Tuple r=sfink

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/634a46da0bdc
https://hg.mozilla.org/mozilla-central/rev/577530c09b82