Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (183973 => 183974)
--- trunk/Source/_javascript_Core/ChangeLog 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-05-08 02:12:35 UTC (rev 183974)
@@ -1,3 +1,55 @@
+2015-05-07 Filip Pizlo <[email protected]>
+
+ GC has trouble with pathologically large array allocations
+ https://bugs.webkit.org/show_bug.cgi?id=144609
+
+ Reviewed by Geoffrey Garen.
+
+ The bug was that SlotVisitor::copyLater() would return early for oversize blocks (right
+ after pinning them), and would skip the accounting. The GC calculates the size of the heap
+ in tandem with the scan to save time, and that accounting was part of how the GC would
+ know how big the heap was. The GC would then think that oversize copied blocks use no
+ memory, and would then mess up its scheduling of the next GC.
+
+ Fixing this bug is harder than it seems. When running an eden GC, we figure out the heap
+ size by summing the size from the last collection and the size by walking the eden heap.
+ But this breaks when we eagerly delete objects that the last collection touched. We can do
+ that in one corner case: copied block reallocation. The old block will be deleted from old
+ space during the realloc and a new block will be allocated in new space. In order for the
+ GC to know that the size of old space actually shrank, we need a field to tell us how much
+ such shrinkage could occur. Since this is a very dirty corner case and it only works for
+ very particular reasons arising from the special properties of copied space (single owner,
+ and the realloc is used in places where the compiler already knows that it cannot register
+ allocate a pointer to the old block), I opted for an equally dirty shrinkage counter
+ devoted just to this case. It's called bytesRemovedFromOldSpaceDueToReallocation.
+
+ To test this, I needed to add an Option to force a particular RAM size in the GC. This
+ allows us to write tests that assert that the GC heap size is some value X, without
+ worrying about machine-to-machine variations due to GC heuristics changing based on RAM
+ size.
+
+ * heap/CopiedSpace.cpp:
+ (JSC::CopiedSpace::CopiedSpace): Initialize the dirty shrinkage counter.
+ (JSC::CopiedSpace::tryReallocateOversize): Bump the dirty shrinkage counter.
+ * heap/CopiedSpace.h:
+ (JSC::CopiedSpace::takeBytesRemovedFromOldSpaceDueToReallocation): Swap out the counter. Used by the GC when it does its accounting.
+ * heap/Heap.cpp:
+ (JSC::Heap::Heap): Allow the user to force the RAM size.
+ (JSC::Heap::updateObjectCounts): Use the dirty shrinkage counter to good effect. Also, 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):
+ * runtime/Options.h:
+ * 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-07 Saam Barati <[email protected]>
Global functions should be initialized as JSFunctions in byte code
Modified: trunk/Source/_javascript_Core/heap/CopiedSpace.cpp (183973 => 183974)
--- trunk/Source/_javascript_Core/heap/CopiedSpace.cpp 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Source/_javascript_Core/heap/CopiedSpace.cpp 2015-05-08 02:12:35 UTC (rev 183974)
@@ -38,6 +38,7 @@
, m_inCopyingPhase(false)
, m_shouldDoCopyPhase(false)
, m_numberOfLoanedBlocks(0)
+ , m_bytesRemovedFromOldSpaceDueToReallocation(0)
{
}
@@ -155,9 +156,13 @@
CopiedBlock* oldBlock = CopiedSpace::blockFor(oldPtr);
if (oldBlock->isOversize()) {
- if (oldBlock->isOld())
+ // FIXME: Eagerly deallocating the old space block probably buys more confusion than
+ // value.
+ // https://bugs.webkit.org/show_bug.cgi?id=144750
+ if (oldBlock->isOld()) {
+ m_bytesRemovedFromOldSpaceDueToReallocation += oldBlock->size();
m_oldGen.oversizeBlocks.remove(oldBlock);
- else
+ } else
m_newGen.oversizeBlocks.remove(oldBlock);
m_blockSet.remove(oldBlock);
CopiedBlock::destroy(oldBlock);
Modified: trunk/Source/_javascript_Core/heap/CopiedSpace.h (183973 => 183974)
--- trunk/Source/_javascript_Core/heap/CopiedSpace.h 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Source/_javascript_Core/heap/CopiedSpace.h 2015-05-08 02:12:35 UTC (rev 183974)
@@ -86,6 +86,13 @@
static CopiedBlock* blockFor(void*);
Heap* heap() const { return m_heap; }
+
+ size_t takeBytesRemovedFromOldSpaceDueToReallocation()
+ {
+ size_t result = 0;
+ std::swap(m_bytesRemovedFromOldSpaceDueToReallocation, result);
+ return result;
+ }
private:
static bool isOversize(size_t);
@@ -135,6 +142,8 @@
Mutex m_loanedBlocksLock;
ThreadCondition m_loanedBlocksCondition;
size_t m_numberOfLoanedBlocks;
+
+ size_t m_bytesRemovedFromOldSpaceDueToReallocation;
static const size_t s_maxAllocationSize = CopiedBlock::blockSize / 2;
static const size_t s_initialBlockNum = 16;
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (183973 => 183974)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2015-05-08 02:12:35 UTC (rev 183974)
@@ -314,7 +314,7 @@
Heap::Heap(VM* vm, HeapType heapType)
: m_heapType(heapType)
- , m_ramSize(ramSize())
+ , m_ramSize(Options::forceRAMSize() ? Options::forceRAMSize() : ramSize())
, m_minBytesPerCycle(minHeapSize(m_heapType, m_ramSize))
, m_sizeAfterLastCollect(0)
, m_sizeAfterLastFullCollect(0)
@@ -818,15 +818,18 @@
#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();
- }
+
+ size_t bytesRemovedFromOldSpaceDueToReallocation =
+ m_storageSpace.takeBytesRemovedFromOldSpaceDueToReallocation();
+
+ if (m_operationInProgress == FullCollection) {
+ m_totalBytesVisited = 0;
+ m_totalBytesCopied = 0;
+ } else
+ m_totalBytesCopied -= bytesRemovedFromOldSpaceDueToReallocation;
+
+ 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 (183973 => 183974)
--- trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h 2015-05-08 02:12:35 UTC (rev 183974)
@@ -239,8 +239,13 @@
ASSERT(bytes);
CopiedBlock* block = CopiedSpace::blockFor(ptr);
if (block->isOversize()) {
+ ASSERT(bytes <= block->size());
+ // FIXME: We should be able to shrink the allocation if bytes went below the block size.
+ // For now, we just make sure that our accounting of how much memory we are actually using
+ // is correct.
+ // https://bugs.webkit.org/show_bug.cgi?id=144749
+ bytes = block->size();
m_shared.m_copiedSpace->pin(block);
- return;
}
ASSERT(heap()->m_storageSpace.contains(block));
Modified: trunk/Source/_javascript_Core/jsc.cpp (183973 => 183974)
--- trunk/Source/_javascript_Core/jsc.cpp 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Source/_javascript_Core/jsc.cpp 2015-05-08 02:12:35 UTC (rev 183974)
@@ -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*);
@@ -586,6 +587,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);
@@ -834,23 +836,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/runtime/Options.h (183973 => 183974)
--- trunk/Source/_javascript_Core/runtime/Options.h 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Source/_javascript_Core/runtime/Options.h 2015-05-08 02:12:35 UTC (rev 183974)
@@ -291,6 +291,7 @@
v(gcLogLevel, logGC, GCLogging::None, "debugging option to log GC activity (0 = None, 1 = Basic, 2 = Verbose)") \
v(bool, disableGC, false, nullptr) \
v(unsigned, gcMaxHeapSize, 0, nullptr) \
+ v(unsigned, forceRAMSize, 0, nullptr) \
v(bool, recordGCPauseTimes, false, nullptr) \
v(bool, logHeapStatisticsAtExit, false, nullptr) \
v(bool, enableTypeProfiler, false, nullptr) \
Modified: trunk/Source/_javascript_Core/tests/stress/new-array-storage-array-with-size.js (183973 => 183974)
--- trunk/Source/_javascript_Core/tests/stress/new-array-storage-array-with-size.js 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Source/_javascript_Core/tests/stress/new-array-storage-array-with-size.js 2015-05-08 02:12:35 UTC (rev 183974)
@@ -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 => 183974)
--- 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-08 02:12:35 UTC (rev 183974)
@@ -0,0 +1,47 @@
+// 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!
+//@ runWithRAMSize(10000000)
+
+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;
+}
+
+var result = gcHeapSize();
+
+for (var i = 0; i < 1000; ++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 3725734 before and 125782 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 > 10000000)
+ throw "Error: heap too big before forced GC: " + result;
+
+// Do a final check after GC, just for sanity.
+gc();
+result = gcHeapSize();
+if (result > 1000000)
+ throw "Error: heap too big after forced GC: " + result;
Modified: trunk/Tools/ChangeLog (183973 => 183974)
--- trunk/Tools/ChangeLog 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Tools/ChangeLog 2015-05-08 02:12:35 UTC (rev 183974)
@@ -1,3 +1,20 @@
+2015-05-07 Filip Pizlo <[email protected]>
+
+ GC has trouble with pathologically large array allocations
+ https://bugs.webkit.org/show_bug.cgi?id=144609
+
+ Reviewed by Geoffrey Garen.
+
+ Add a --filter option that restricts the set of tests we run. I needed it to fix this bug
+ and it's a frequently requested feature.
+
+ Also add the ability to run a test pretending that your system has a particular RAM size.
+ This is useful for GC tests, and the new GC test that I added uses this.
+
+ * Scripts/run-_javascript_core-tests:
+ (runJSCStressTests):
+ * Scripts/run-jsc-stress-tests:
+
2015-05-07 Csaba Osztrogonác <[email protected]>
[EFL] Bump EFL version to 1.14.0
Modified: trunk/Tools/Scripts/run-_javascript_core-tests (183973 => 183974)
--- trunk/Tools/Scripts/run-_javascript_core-tests 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Tools/Scripts/run-_javascript_core-tests 2015-05-08 02:12:35 UTC (rev 183974)
@@ -66,6 +66,7 @@
my $buildJSCDefault = $buildJSC ? "will check" : "will not check";
my $testapiDefault = $runTestAPI ? "will run" : "will not run";
my $jscStressDefault = $runJSCStress ? "will run" : " will not run";
+my $filter;
my $usage = <<EOF;
Usage: $programName [options] [options to pass to build system]
--help Show this help message
@@ -89,6 +90,7 @@
--shell-runner Uses the shell-based test runner instead of the default make-based runner.
In general the shell runner is slower than the make runner.
--make-runner Uses the faster make-based runner.
+ --filter Only run tests whose name matches the given regular _expression_.
EOF
@@ -106,6 +108,7 @@
'child-processes=s' => \$childProcesses,
'shell-runner' => \$shellRunner,
'make-runner' => \$makeRunner,
+ 'filter=s' => \$filter,
'help' => \$showHelp
);
@@ -313,6 +316,11 @@
if ($makeRunner) {
push(@jscStressDriverCmd, "--make-runner");
}
+
+ if ($filter) {
+ push(@jscStressDriverCmd, "--filter");
+ push(@jscStressDriverCmd, $filter);
+ }
# End option processing, the rest of the arguments are tests
push(@jscStressDriverCmd, "--");
Modified: trunk/Tools/Scripts/run-jsc-stress-tests (183973 => 183974)
--- trunk/Tools/Scripts/run-jsc-stress-tests 2015-05-08 02:05:15 UTC (rev 183973)
+++ trunk/Tools/Scripts/run-jsc-stress-tests 2015-05-08 02:12:35 UTC (rev 183974)
@@ -105,6 +105,7 @@
$remoteDirectory = nil
$architecture = nil
$hostOS = nil
+$filter = nil
def usage
@@ -130,6 +131,7 @@
puts "--remote Specify a remote host on which to run tests from command line argument."
puts "--remote-config-file Specify a remote host on which to run tests from JSON file."
puts "--child-processes (-c) Specify the number of child processes."
+ puts "--filter Only run tests whose name matches the given regular _expression_."
puts "--help (-h) Print this message."
exit 1
end
@@ -152,6 +154,7 @@
['--remote', GetoptLong::REQUIRED_ARGUMENT],
['--remote-config-file', GetoptLong::REQUIRED_ARGUMENT],
['--child-processes', '-c', GetoptLong::REQUIRED_ARGUMENT],
+ ['--filter', GetoptLong::REQUIRED_ARGUMENT],
['--verbose', '-v', GetoptLong::NO_ARGUMENT]).each {
| opt, arg |
case opt
@@ -191,6 +194,8 @@
$remoteConfigFile = arg
when '--child-processes'
$numChildProcesses = arg.to_i
+ when '--filter'
+ $filter = Regexp.new(arg)
when '--arch'
$architecture = arg
when '--os'
@@ -594,7 +599,11 @@
def addRunCommand(kind, command, outputHandler, errorHandler)
$didAddRunCommand = true
- plan = Plan.new($benchmarkDirectory, command, baseOutputName(kind), outputHandler, errorHandler)
+ name = baseOutputName(kind)
+ if $filter and name !~ $filter
+ return
+ end
+ plan = Plan.new($benchmarkDirectory, command, name, outputHandler, errorHandler)
if $numChildProcesses > 1 and $runCommandOptions[:isSlow]
$runlist.unshift plan
else
@@ -644,6 +653,10 @@
run("default")
end
+def runWithRAMSize(size)
+ run("ram-size-#{size}", "--forceRAMSize=#{size}")
+end
+
def runNoLLInt
run("no-llint", "--useLLInt=false")
end
