⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 183974 in webkit


Ignore:
Timestamp:
May 7, 2015, 7:12:35 PM (11 years ago)
Author:
fpizlo@apple.com
Message:

GC has trouble with pathologically large array allocations
​https://bugs.webkit.org/show_bug.cgi?id=144609

Reviewed by Geoffrey Garen.
Source/JavaScriptCore:

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

Tools:


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-javascriptcore-tests:

(runJSCStressTests):

  • Scripts/run-jsc-stress-tests:
Location:
trunk
Files:
1 added
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r183972 r183974  
     12015-05-07  Filip Pizlo  <fpizlo@apple.com>
     2
     3        GC has trouble with pathologically large array allocations
     4        https://bugs.webkit.org/show_bug.cgi?id=144609
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        The bug was that SlotVisitor::copyLater() would return early for oversize blocks (right
     9        after pinning them), and would skip the accounting. The GC calculates the size of the heap
     10        in tandem with the scan to save time, and that accounting was part of how the GC would
     11        know how big the heap was. The GC would then think that oversize copied blocks use no
     12        memory, and would then mess up its scheduling of the next GC.
     13       
     14        Fixing this bug is harder than it seems. When running an eden GC, we figure out the heap
     15        size by summing the size from the last collection and the size by walking the eden heap.
     16        But this breaks when we eagerly delete objects that the last collection touched. We can do
     17        that in one corner case: copied block reallocation. The old block will be deleted from old
     18        space during the realloc and a new block will be allocated in new space. In order for the
     19        GC to know that the size of old space actually shrank, we need a field to tell us how much
     20        such shrinkage could occur. Since this is a very dirty corner case and it only works for
     21        very particular reasons arising from the special properties of copied space (single owner,
     22        and the realloc is used in places where the compiler already knows that it cannot register
     23        allocate a pointer to the old block), I opted for an equally dirty shrinkage counter
     24        devoted just to this case. It's called bytesRemovedFromOldSpaceDueToReallocation.
     25       
     26        To test this, I needed to add an Option to force a particular RAM size in the GC. This
     27        allows us to write tests that assert that the GC heap size is some value X, without
     28        worrying about machine-to-machine variations due to GC heuristics changing based on RAM
     29        size.
     30
     31        * heap/CopiedSpace.cpp:
     32        (JSC::CopiedSpace::CopiedSpace): Initialize the dirty shrinkage counter.
     33        (JSC::CopiedSpace::tryReallocateOversize): Bump the dirty shrinkage counter.
     34        * heap/CopiedSpace.h:
     35        (JSC::CopiedSpace::takeBytesRemovedFromOldSpaceDueToReallocation): Swap out the counter. Used by the GC when it does its accounting.
     36        * heap/Heap.cpp:
     37        (JSC::Heap::Heap): Allow the user to force the RAM size.
     38        (JSC::Heap::updateObjectCounts): Use the dirty shrinkage counter to good effect. Also, make this code less confusing.
     39        * heap/SlotVisitorInlines.h:
     40        (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.
     41        * jsc.cpp: Add size measuring hooks to write the largeish test.
     42        (GlobalObject::finishCreation):
     43        (functionGCAndSweep):
     44        (functionFullGC):
     45        (functionEdenGC):
     46        (functionHeapSize):
     47        * runtime/Options.h:
     48        * 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.
     49        * 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.
     50        (foo):
     51        (test):
     52
    1532015-05-07  Saam Barati  <saambarati1@gmail.com>
    254

  • trunk/Source/JavaScriptCore/heap/CopiedSpace.cpp

    r181485 r183974  
    3939    , m_shouldDoCopyPhase(false)
    4040    , m_numberOfLoanedBlocks(0)
     41    , m_bytesRemovedFromOldSpaceDueToReallocation(0)
    4142{
    4243}
    … …  
    156157    CopiedBlock* oldBlock = CopiedSpace::blockFor(oldPtr);
    157158    if (oldBlock->isOversize()) {
    158         if (oldBlock->isOld())
     159        // FIXME: Eagerly deallocating the old space block probably buys more confusion than
     160        // value.
     161        // https://bugs.webkit.org/show_bug.cgi?id=144750
     162        if (oldBlock->isOld()) {
     163            m_bytesRemovedFromOldSpaceDueToReallocation += oldBlock->size();
    159164            m_oldGen.oversizeBlocks.remove(oldBlock);
    160         else
     165        } else
    161166            m_newGen.oversizeBlocks.remove(oldBlock);
    162167        m_blockSet.remove(oldBlock);

  • trunk/Source/JavaScriptCore/heap/CopiedSpace.h

    r181758 r183974  
    8787
    8888    Heap* heap() const { return m_heap; }
     89   
     90    size_t takeBytesRemovedFromOldSpaceDueToReallocation()
     91    {
     92        size_t result = 0;
     93        std::swap(m_bytesRemovedFromOldSpaceDueToReallocation, result);
     94        return result;
     95    }
    8996
    9097private:
    … …  
    136143    ThreadCondition m_loanedBlocksCondition;
    137144    size_t m_numberOfLoanedBlocks;
     145   
     146    size_t m_bytesRemovedFromOldSpaceDueToReallocation;
    138147
    139148    static const size_t s_maxAllocationSize = CopiedBlock::blockSize / 2;

  • trunk/Source/JavaScriptCore/heap/Heap.cpp

    r183938 r183974  
    315315Heap::Heap(VM* vm, HeapType heapType)
    316316    : m_heapType(heapType)
    317     , m_ramSize(ramSize())
     317    , m_ramSize(Options::forceRAMSize() ? Options::forceRAMSize() : ramSize())
    318318    , m_minBytesPerCycle(minHeapSize(m_heapType, m_ramSize))
    319319    , m_sizeAfterLastCollect(0)
    … …  
    819819        dataLogF("\nNumber of live Objects after GC %lu, took %.6f secs\n", static_cast<unsigned long>(visitCount), WTF::monotonicallyIncreasingTime() - gcStartTime);
    820820    }
    821 
    822     if (m_operationInProgress == EdenCollection) {
    823         m_totalBytesVisited += m_slotVisitor.bytesVisited();
    824         m_totalBytesCopied += m_slotVisitor.bytesCopied();
    825     } else {
    826         ASSERT(m_operationInProgress == FullCollection);
    827         m_totalBytesVisited = m_slotVisitor.bytesVisited();
    828         m_totalBytesCopied = m_slotVisitor.bytesCopied();
    829     }
     821   
     822    size_t bytesRemovedFromOldSpaceDueToReallocation =
     823        m_storageSpace.takeBytesRemovedFromOldSpaceDueToReallocation();
     824   
     825    if (m_operationInProgress == FullCollection) {
     826        m_totalBytesVisited = 0;
     827        m_totalBytesCopied = 0;
     828    } else
     829        m_totalBytesCopied -= bytesRemovedFromOldSpaceDueToReallocation;
     830   
     831    m_totalBytesVisited += m_slotVisitor.bytesVisited();
     832    m_totalBytesCopied += m_slotVisitor.bytesCopied();
    830833#if ENABLE(PARALLEL_GC)
    831834    m_totalBytesVisited += m_sharedData.childBytesVisited();

  • trunk/Source/JavaScriptCore/heap/SlotVisitorInlines.h

    r183872 r183974  
    240240    CopiedBlock* block = CopiedSpace::blockFor(ptr);
    241241    if (block->isOversize()) {
     242        ASSERT(bytes <= block->size());
     243        // FIXME: We should be able to shrink the allocation if bytes went below the block size.
     244        // For now, we just make sure that our accounting of how much memory we are actually using
     245        // is correct.
     246        // https://bugs.webkit.org/show_bug.cgi?id=144749
     247        bytes = block->size();
    242248        m_shared.m_copiedSpace->pin(block);
    243         return;
    244249    }
    245250

  • trunk/Source/JavaScriptCore/jsc.cpp

    r183962 r183974  
    448448static EncodedJSValue JSC_HOST_CALL functionFullGC(ExecState*);
    449449static EncodedJSValue JSC_HOST_CALL functionEdenGC(ExecState*);
     450static EncodedJSValue JSC_HOST_CALL functionHeapSize(ExecState*);
    450451static EncodedJSValue JSC_HOST_CALL functionDeleteAllCompiledCode(ExecState*);
    451452#ifndef NDEBUG
    … …  
    587588        addFunction(vm, "fullGC", functionFullGC, 0);
    588589        addFunction(vm, "edenGC", functionEdenGC, 0);
     590        addFunction(vm, "gcHeapSize", functionHeapSize, 0);
    589591        addFunction(vm, "deleteAllCompiledCode", functionDeleteAllCompiledCode, 0);
    590592#ifndef NDEBUG
    … …  
    835837    JSLockHolder lock(exec);
    836838    exec->heap()->collectAllGarbage();
    837     return JSValue::encode(jsUndefined());
     839    return JSValue::encode(jsNumber(exec->heap()->sizeAfterLastFullCollection()));
    838840}
    839841
    … …  
    842844    JSLockHolder lock(exec);
    843845    exec->heap()->collect(FullCollection);
    844     return JSValue::encode(jsUndefined());
     846    return JSValue::encode(jsNumber(exec->heap()->sizeAfterLastFullCollection()));
    845847}
    846848
    … …  
    849851    JSLockHolder lock(exec);
    850852    exec->heap()->collect(EdenCollection);
    851     return JSValue::encode(jsUndefined());
     853    return JSValue::encode(jsNumber(exec->heap()->sizeAfterLastEdenCollection()));
     854}
     855
     856EncodedJSValue JSC_HOST_CALL functionHeapSize(ExecState* exec)
     857{
     858    JSLockHolder lock(exec);
     859    return JSValue::encode(jsNumber(exec->heap()->size()));
    852860}
    853861

  • trunk/Source/JavaScriptCore/runtime/Options.h

    r183072 r183974  
    292292    v(bool, disableGC, false, nullptr) \
    293293    v(unsigned, gcMaxHeapSize, 0, nullptr) \
     294    v(unsigned, forceRAMSize, 0, nullptr) \
    294295    v(bool, recordGCPauseTimes, false, nullptr) \
    295296    v(bool, logHeapStatisticsAtExit, false, nullptr) \

  • trunk/Source/JavaScriptCore/tests/stress/new-array-storage-array-with-size.js

    r183872 r183974  
    1 // https://bugs.webkit.org/show_bug.cgi?id=144609
    2 //@ skip
    3 
    41function foo(x) {
    52    return new Array(x);
    … …  
    74
    85noInline(foo);
     6
     7// Warm up up to create array storage.
     8for (var i = 0; i < 10000; ++i) {
     9    var array = foo(10);
     10    array.__defineSetter__(0, function(v) { });
     11}
    912
    1013function test(size) {
    … …  
    2326
    2427for (var i = 0; i < 100000; ++i) {
    25     test(1000000);
     28    test(10);
    2629}

  • trunk/Tools/ChangeLog

    r183973 r183974  
     12015-05-07  Filip Pizlo  <fpizlo@apple.com>
     2
     3        GC has trouble with pathologically large array allocations
     4        https://bugs.webkit.org/show_bug.cgi?id=144609
     5
     6        Reviewed by Geoffrey Garen.
     7       
     8        Add a --filter option that restricts the set of tests we run. I needed it to fix this bug
     9        and it's a frequently requested feature.
     10       
     11        Also add the ability to run a test pretending that your system has a particular RAM size.
     12        This is useful for GC tests, and the new GC test that I added uses this.
     13
     14        * Scripts/run-javascriptcore-tests:
     15        (runJSCStressTests):
     16        * Scripts/run-jsc-stress-tests:
     17
    1182015-05-07  Csaba Osztrogonác  <ossy@webkit.org>
    219

  • trunk/Tools/Scripts/run-javascriptcore-tests

    r183593 r183974  
    6767my $testapiDefault = $runTestAPI ? "will run" : "will not run";
    6868my $jscStressDefault = $runJSCStress ? "will run" : " will not run";
     69my $filter;
    6970my $usage = <<EOF;
    7071Usage: $programName [options] [options to pass to build system]
    … …  
    9091                                In general the shell runner is slower than the make runner.
    9192  --make-runner                 Uses the faster make-based runner.
     93  --filter                      Only run tests whose name matches the given regular expression.
    9294
    9395EOF
    … …  
    107109    'shell-runner' => \$shellRunner,
    108110    'make-runner' => \$makeRunner,
     111    'filter=s' => \$filter,
    109112    'help' => \$showHelp
    110113);
    … …  
    313316    if ($makeRunner) {
    314317        push(@jscStressDriverCmd, "--make-runner");
     318    }
     319   
     320    if ($filter) {
     321        push(@jscStressDriverCmd, "--filter");
     322        push(@jscStressDriverCmd, $filter);
    315323    }
    316324

  • trunk/Tools/Scripts/run-jsc-stress-tests

    r182332 r183974  
    106106$architecture = nil
    107107$hostOS = nil
     108$filter = nil
    108109
    109110
    … …  
    131132    puts "--remote-config-file        Specify a remote host on which to run tests from JSON file."
    132133    puts "--child-processes    (-c)   Specify the number of child processes."
     134    puts "--filter                    Only run tests whose name matches the given regular expression."
    133135    puts "--help               (-h)   Print this message."
    134136    exit 1
    … …  
    153155               ['--remote-config-file', GetoptLong::REQUIRED_ARGUMENT],
    154156               ['--child-processes', '-c', GetoptLong::REQUIRED_ARGUMENT],
     157               ['--filter', GetoptLong::REQUIRED_ARGUMENT],
    155158               ['--verbose', '-v', GetoptLong::NO_ARGUMENT]).each {
    156159    | opt, arg |
    … …  
    192195    when '--child-processes'
    193196        $numChildProcesses = arg.to_i
     197    when '--filter'
     198        $filter = Regexp.new(arg)
    194199    when '--arch'
    195200        $architecture = arg
    … …  
    595600def addRunCommand(kind, command, outputHandler, errorHandler)
    596601    $didAddRunCommand = true
    597     plan = Plan.new($benchmarkDirectory, command, baseOutputName(kind), outputHandler, errorHandler)
     602    name = baseOutputName(kind)
     603    if $filter and name !~ $filter
     604        return
     605    end
     606    plan = Plan.new($benchmarkDirectory, command, name, outputHandler, errorHandler)
    598607    if $numChildProcesses > 1 and $runCommandOptions[:isSlow]
    599608        $runlist.unshift plan
    … …  
    643652def runDefault
    644653    run("default")
     654end
     655
     656def runWithRAMSize(size)
     657    run("ram-size-#{size}", "--forceRAMSize=#{size}")
    645658end
    646659
Note: See TracChangeset for help on using the changeset viewer.