GC has trouble with pathologically large array allocations

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Consider the following test, if you tweak the size so that we actually allocate the memory.  It causes linear heap growth.


function foo(x) {
    return new Array(x);
}

noInline(foo);

function test(size) {
    var result = foo(size);
    if (result.length != size)
        throw "Error: bad result: " + result;
    var sawThings = false;
    for (var s in result)
        sawThings = true;
    if (sawThings)
        throw "Error: array is in bad state: " + result;
    result[0] = "42.5";
    if (result[0] != "42.5")
        throw "Error: array is in wierd state: " + result;
}

for (var i = 0; i < 100000; ++i) {
    test(1000000);
}

[Filip Pizlo]

Created attachment 252432 [details]
the patch

[Mark Lam]

Comment on attachment 252432 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252432&action=review

r=me

> Source/JavaScriptCore/tests/stress/new-largeish-contiguous-array-with-size.js:23
> +        throw "Error: array is in wierd state: " + result;

I believe the correct spelling here is "weird".  Can you fix this and the one in new-array-storage-array-with-size.js as well?

> Source/JavaScriptCore/tests/stress/new-largeish-contiguous-array-with-size.js:28
> +for (var i = 0; i < 40000; ++i) {
> +    test(50000);
> +}

Please add a comment above this to indicate that if GC doesn't clean up adequately, then the heap can grow unreasonably large (in the order of 40000 * 50000 * 8), and would end up much larger than the 100M tested below.  While this purpose can be inferred, it is not clear.  I think a comment will make it explicit and help the reader see right away what the test is trying to check for.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/183847

[Csaba Osztrogonác]

(In reply to comment #4)
> Landed in http://trac.webkit.org/changeset/183847

It caused assertions on the debug bots. Could you check it?

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > Landed in http://trac.webkit.org/changeset/183847
> 
> It caused assertions on the debug bots. Could you check it?

See: https://bugs.webkit.org/show_bug.cgi?id=144667

Should be fixed in http://trac.webkit.org/changeset/183851

[Csaba Osztrogonác]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Landed in http://trac.webkit.org/changeset/183847
> > 
> > It caused assertions on the debug bots. Could you check it?
> 
> See: https://bugs.webkit.org/show_bug.cgi?id=144667
> 
> Should be fixed in http://trac.webkit.org/changeset/183851

Yes, it fixed the assertions, thanks.

But unfortunately the new test fails on 32 bit ARM Linux bots:

stress/new-largeish-contiguous-array-with-size.js.default: Exception: Error: heap too big before forced GC: 129308808
stress/new-largeish-contiguous-array-with-size.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/new-largeish-contiguous-array-with-size.js.default

stress/new-largeish-contiguous-array-with-size.js.default: Exception: Error: heap too big before forced GC: 130977916
stress/new-largeish-contiguous-array-with-size.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/new-largeish-contiguous-array-with-size.js.default

[Csaba Osztrogonác]

And there are still 25-30 layout test assertions.

[Alexey Proskuryakov]

E.g. this one: <https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fworkers%2Fworker-gc2.html>.

Rolling out (Csaba, why didn't you?)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 144691

[Alexey Proskuryakov]

There are also assorted JSC test failures. Hard to tell if they are all caused by this patch, there was a lot of churn last night.

https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20JSC%20%28Tests%29/builds/1241/steps/jscore-test/logs/stdio

https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/66067/steps/jscore-test/logs/stdio

[Filip Pizlo]

Man, this bug is turning out to be brutal.  It appears that an old gen block is revived into new space and then deleted.  But because we haven't done a full collection yet, our running total of blocks includes that block.  Hence the assertion failure.

All of this feels very wrong.

[Filip Pizlo]

Aha!  We are resizing blocks.  This means that our count of the size of old space is wrong, because we will double-count: we will count both the block we deleted and the block we created.

[Csaba Osztrogonác]

(In reply to comment #9)
> Rolling out (Csaba, why didn't you?)

I was the bad guy for too long and I was fed up with getting angry comments,
so I promised not to roll out others patches even if it broke the whole world.

Patch owners are responsible for their patches, they should respect the EWS
bubbles, watch the bots after landing. I won't argue with anybody and won't
ask anybody to respect the policies.

[Filip Pizlo]

Created attachment 252599 [details]
the patch

[Geoffrey Garen]

Comment on attachment 252599 [details]
the patch

I'm OK with this solution. But I also think we should just remove that immediate deallocation feature. That's just not how garbage collection works, and making it work that way some small percentage of the time doesn't strike me as valuable.

[Filip Pizlo]

*** Bug 144677 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

(In reply to comment #16)
> Comment on attachment 252599 [details]
> the patch
> 
> I'm OK with this solution. But I also think we should just remove that
> immediate deallocation feature. That's just not how garbage collection
> works, and making it work that way some small percentage of the time doesn't
> strike me as valuable.

I agree. Filed: https://bugs.webkit.org/show_bug.cgi?id=144750

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/183974