- Revision
- 183847
- Author
- [email protected]
- Date
- 2015-05-05 20:39:20 -0700 (Tue, 05 May 2015)
Log Message
GC has trouble with pathologically large array allocations
https://bugs.webkit.org/show_bug.cgi?id=144609
Reviewed by Mark Lam.
* heap/Heap.cpp:
(JSC::Heap::updateObjectCounts): Make this code less confusing.
* heap/SlotVisitorInlines.h:
(JSC::SlotVisitor::copyLater): The early return for isOversize() was the bug. We still need to report these bytes as live. Otherwise the GC doesn't know that it owns this memory.
* jsc.cpp: Add size measuring hooks to write the largeish test.
(GlobalObject::finishCreation):
(functionGCAndSweep):
(functionFullGC):
(functionEdenGC):
(functionHeapSize):
* tests/stress/new-array-storage-array-with-size.js: Fix this so that it actually allocates ArrayStorage arrays and tests the thing it was supposed to test.
* tests/stress/new-largeish-contiguous-array-with-size.js: Added. This tests what the other test accidentally started testing, but does so without running your system out of memory.
(foo):
(test):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (183846 => 183847)
--- trunk/Source/_javascript_Core/ChangeLog 2015-05-06 02:10:43 UTC (rev 183846)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-05-06 03:39:20 UTC (rev 183847)
@@ -1,5 +1,27 @@
2015-05-05 Filip Pizlo <[email protected]>
+ GC has trouble with pathologically large array allocations
+ https://bugs.webkit.org/show_bug.cgi?id=144609
+
+ Reviewed by Mark Lam.
+
+ * heap/Heap.cpp:
+ (JSC::Heap::updateObjectCounts): Make this code less confusing.
+ * heap/SlotVisitorInlines.h:
+ (JSC::SlotVisitor::copyLater): The early return for isOversize() was the bug. We still need to report these bytes as live. Otherwise the GC doesn't know that it owns this memory.
+ * jsc.cpp: Add size measuring hooks to write the largeish test.
+ (GlobalObject::finishCreation):
+ (functionGCAndSweep):
+ (functionFullGC):
+ (functionEdenGC):
+ (functionHeapSize):
+ * tests/stress/new-array-storage-array-with-size.js: Fix this so that it actually allocates ArrayStorage arrays and tests the thing it was supposed to test.
+ * tests/stress/new-largeish-contiguous-array-with-size.js: Added. This tests what the other test accidentally started testing, but does so without running your system out of memory.
+ (foo):
+ (test):
+
+2015-05-05 Filip Pizlo <[email protected]>
+
FTL SwitchString slow case creates duplicate switch cases
https://bugs.webkit.org/show_bug.cgi?id=144634
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (183846 => 183847)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2015-05-06 02:10:43 UTC (rev 183846)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2015-05-06 03:39:20 UTC (rev 183847)
@@ -816,15 +816,14 @@
#endif
dataLogF("\nNumber of live Objects after GC %lu, took %.6f secs\n", static_cast<unsigned long>(visitCount), WTF::monotonicallyIncreasingTime() - gcStartTime);
}
-
- if (m_operationInProgress == EdenCollection) {
- m_totalBytesVisited += m_slotVisitor.bytesVisited();
- m_totalBytesCopied += m_slotVisitor.bytesCopied();
- } else {
- ASSERT(m_operationInProgress == FullCollection);
- m_totalBytesVisited = m_slotVisitor.bytesVisited();
- m_totalBytesCopied = m_slotVisitor.bytesCopied();
+
+ if (m_operationInProgress == FullCollection) {
+ m_totalBytesVisited = 0;
+ m_totalBytesCopied = 0;
}
+
+ m_totalBytesVisited += m_slotVisitor.bytesVisited();
+ m_totalBytesCopied += m_slotVisitor.bytesCopied();
#if ENABLE(PARALLEL_GC)
m_totalBytesVisited += m_sharedData.childBytesVisited();
m_totalBytesCopied += m_sharedData.childBytesCopied();
Modified: trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h (183846 => 183847)
--- trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h 2015-05-06 02:10:43 UTC (rev 183846)
+++ trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h 2015-05-06 03:39:20 UTC (rev 183847)
@@ -238,10 +238,8 @@
{
ASSERT(bytes);
CopiedBlock* block = CopiedSpace::blockFor(ptr);
- if (block->isOversize()) {
+ if (block->isOversize())
m_shared.m_copiedSpace->pin(block);
- return;
- }
ASSERT(heap()->m_storageSpace.contains(block));
Modified: trunk/Source/_javascript_Core/jsc.cpp (183846 => 183847)
--- trunk/Source/_javascript_Core/jsc.cpp 2015-05-06 02:10:43 UTC (rev 183846)
+++ trunk/Source/_javascript_Core/jsc.cpp 2015-05-06 03:39:20 UTC (rev 183847)
@@ -447,6 +447,7 @@
static EncodedJSValue JSC_HOST_CALL functionGCAndSweep(ExecState*);
static EncodedJSValue JSC_HOST_CALL functionFullGC(ExecState*);
static EncodedJSValue JSC_HOST_CALL functionEdenGC(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionHeapSize(ExecState*);
static EncodedJSValue JSC_HOST_CALL functionDeleteAllCompiledCode(ExecState*);
#ifndef NDEBUG
static EncodedJSValue JSC_HOST_CALL functionReleaseExecutableMemory(ExecState*);
@@ -585,6 +586,7 @@
addFunction(vm, "gc", functionGCAndSweep, 0);
addFunction(vm, "fullGC", functionFullGC, 0);
addFunction(vm, "edenGC", functionEdenGC, 0);
+ addFunction(vm, "gcHeapSize", functionHeapSize, 0);
addFunction(vm, "deleteAllCompiledCode", functionDeleteAllCompiledCode, 0);
#ifndef NDEBUG
addFunction(vm, "dumpCallFrame", functionDumpCallFrame, 0);
@@ -831,23 +833,29 @@
{
JSLockHolder lock(exec);
exec->heap()->collectAllGarbage();
- return JSValue::encode(jsUndefined());
+ return JSValue::encode(jsNumber(exec->heap()->sizeAfterLastFullCollection()));
}
EncodedJSValue JSC_HOST_CALL functionFullGC(ExecState* exec)
{
JSLockHolder lock(exec);
exec->heap()->collect(FullCollection);
- return JSValue::encode(jsUndefined());
+ return JSValue::encode(jsNumber(exec->heap()->sizeAfterLastFullCollection()));
}
EncodedJSValue JSC_HOST_CALL functionEdenGC(ExecState* exec)
{
JSLockHolder lock(exec);
exec->heap()->collect(EdenCollection);
- return JSValue::encode(jsUndefined());
+ return JSValue::encode(jsNumber(exec->heap()->sizeAfterLastEdenCollection()));
}
+EncodedJSValue JSC_HOST_CALL functionHeapSize(ExecState* exec)
+{
+ JSLockHolder lock(exec);
+ return JSValue::encode(jsNumber(exec->heap()->size()));
+}
+
EncodedJSValue JSC_HOST_CALL functionDeleteAllCompiledCode(ExecState* exec)
{
JSLockHolder lock(exec);
Modified: trunk/Source/_javascript_Core/tests/stress/new-array-storage-array-with-size.js (183846 => 183847)
--- trunk/Source/_javascript_Core/tests/stress/new-array-storage-array-with-size.js 2015-05-06 02:10:43 UTC (rev 183846)
+++ trunk/Source/_javascript_Core/tests/stress/new-array-storage-array-with-size.js 2015-05-06 03:39:20 UTC (rev 183847)
@@ -1,12 +1,15 @@
-// https://bugs.webkit.org/show_bug.cgi?id=144609
-//@ skip
-
function foo(x) {
return new Array(x);
}
noInline(foo);
+// Warm up up to create array storage.
+for (var i = 0; i < 10000; ++i) {
+ var array = foo(10);
+ array.__defineSetter__(0, function(v) { });
+}
+
function test(size) {
var result = foo(size);
if (result.length != size)
@@ -22,5 +25,5 @@
}
for (var i = 0; i < 100000; ++i) {
- test(1000000);
+ test(10);
}
Added: trunk/Source/_javascript_Core/tests/stress/new-largeish-contiguous-array-with-size.js (0 => 183847)
--- trunk/Source/_javascript_Core/tests/stress/new-largeish-contiguous-array-with-size.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/new-largeish-contiguous-array-with-size.js 2015-05-06 03:39:20 UTC (rev 183847)
@@ -0,0 +1,45 @@
+// We only need one run of this with any GC or JIT strategy. This test is not particularly fast.
+// Unfortunately, it needs to run for a while to test the thing it's testing.
+//@ slow!
+//@ runDefault
+
+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 weird state: " + result;
+}
+
+for (var i = 0; i < 40000; ++i) {
+ // The test was written when we found that large array allocations weren't being accounted for
+ // in that part of the GC's accounting that determined the GC trigger. Consequently, the GC
+ // would run too infrequently in this loop and we would use an absurd amount of memory when this
+ // loop exited.
+ test(50000);
+}
+
+// Last time I tested, the heap should be 52520914 before and 50120146 after. I don't want to
+// enforce exactly that. If you regress the accounting code, the GC heap size at this point will
+// be much more than that.
+var result = gcHeapSize();
+if (result > 100000000)
+ throw "Error: heap too big before forced GC: " + result;
+
+// Do a final check after GC, just for sanity.
+gc();
+result = gcHeapSize();
+if (result > 100000000)
+ throw "Error: heap too big after forced GC: " + result;
