Bug 144609

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

Created attachment 252432 [details] the patch

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.

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

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

(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

(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

And there are still 25-30 layout test assertions.

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

Re-opened since this is blocked by bug 144691

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

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.

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.

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

Created attachment 252599 [details] the patch

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.

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

(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

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