Changeset 263674 in webkit


Ignore:
Timestamp:
Jun 29, 2020 12:07:25 PM (4 years ago)
Author:
keith_miller@apple.com
Message:

ConservativeRoots should mark any cell it finds an interior pointer to
​https://bugs.webkit.org/show_bug.cgi?id=213686

Reviewed by Yusuke Suzuki.

Currently, if ConserativeRoots finds an interior pointer to a cell
it will only mark that cell if it's a butterfly of some
kind. However, this can cause problems if the C++ or B3 compilers
pre-compute the offset of some cell member they want to load from
after a call. If this happens and that interior pointer is the
only reference to the cell it can get collected while it is still
"alive".

A naive patch that doesn't return from
findGCObjectPointersForMarking after finding a live non-interior,
non-butterfly cell was a 2% regression on Speedometer2 and
JetStream2. So, this patch immediately returns after
marking some non-butterfly cell, which appears to have fixed the
regression locally. Given this was such a big regression (likely
from running MarkedBlock::isLive) more than once there's possibly
an optimization opportunity here. I filed
​https://bugs.webkit.org/show_bug.cgi?id=213687 to investigate
further.

  • heap/HeapCell.cpp:

(WTF::printInternal):

  • heap/HeapCell.h:

(JSC::isJSCellKind):
(JSC::mayHaveIndexingHeader):
(JSC::hasInteriorPointers): Deleted.

  • heap/HeapUtil.h:

(JSC::HeapUtil::findGCObjectPointersForMarking):

  • heap/SlotVisitor.cpp:

(JSC::SlotVisitor::appendJSCellOrAuxiliary):

  • runtime/VM.cpp:

(JSC::VM::VM):

Location:
trunk/Source/JavaScriptCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r263635 r263674  
     12020-06-29  Keith Miller  <keith_miller@apple.com>
     2
     3        ConservativeRoots should mark any cell it finds an interior pointer to
     4        https://bugs.webkit.org/show_bug.cgi?id=213686
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        Currently, if ConserativeRoots finds an interior pointer to a cell
     9        it will only mark that cell if it's a butterfly of some
     10        kind. However, this can cause problems if the C++ or B3 compilers
     11        pre-compute the offset of some cell member they want to load from
     12        after a call. If this happens and that interior pointer is the
     13        only reference to the cell it can get collected while it is still
     14        "alive".
     15
     16        A naive patch that doesn't return from
     17        findGCObjectPointersForMarking after finding a live non-interior,
     18        non-butterfly cell was a 2% regression on Speedometer2 and
     19        JetStream2. So, this patch immediately returns after
     20        marking some non-butterfly cell, which appears to have fixed the
     21        regression locally. Given this was such a big regression (likely
     22        from running MarkedBlock::isLive) more than once there's possibly
     23        an optimization opportunity here. I filed
     24        https://bugs.webkit.org/show_bug.cgi?id=213687 to investigate
     25        further.
     26
     27        * heap/HeapCell.cpp:
     28        (WTF::printInternal):
     29        * heap/HeapCell.h:
     30        (JSC::isJSCellKind):
     31        (JSC::mayHaveIndexingHeader):
     32        (JSC::hasInteriorPointers): Deleted.
     33        * heap/HeapUtil.h:
     34        (JSC::HeapUtil::findGCObjectPointersForMarking):
     35        * heap/SlotVisitor.cpp:
     36        (JSC::SlotVisitor::appendJSCellOrAuxiliary):
     37        * runtime/VM.cpp:
     38        (JSC::VM::VM):
     39
    1402020-06-28  Geoffrey Garen  <ggaren@apple.com>
    241

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

    r259547 r263674  
    5555        out.print("JSCell");
    5656        return;
    57     case HeapCell::JSCellWithInteriorPointers:
    58         out.print("JSCellWithInteriorPointers");
     57    case HeapCell::JSCellWithIndexingHeader:
     58        out.print("JSCellWithIndexingHeader");
    5959        return;
    6060    case HeapCell::Auxiliary:

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

    r259547 r263674  
    4343    enum Kind : int8_t {
    4444        JSCell,
    45         JSCellWithInteriorPointers,
     45        JSCellWithIndexingHeader,
    4646        Auxiliary
    4747    };
    … …  
    9494inline bool isJSCellKind(HeapCell::Kind kind)
    9595{
    96     return kind == HeapCell::JSCell || kind == HeapCell::JSCellWithInteriorPointers;
     96    return kind == HeapCell::JSCell || kind == HeapCell::JSCellWithIndexingHeader;
    9797}
    9898
    99 inline bool hasInteriorPointers(HeapCell::Kind kind)
     99inline bool mayHaveIndexingHeader(HeapCell::Kind kind)
    100100{
    101     return kind == HeapCell::Auxiliary || kind == HeapCell::JSCellWithInteriorPointers;
     101    return kind == HeapCell::Auxiliary || kind == HeapCell::JSCellWithIndexingHeader;
    102102}
    103103

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

    r254023 r263674  
    8989            if (!filter.ruleOut(bitwise_cast<Bits>(previousCandidate))
    9090                && set.contains(previousCandidate)
    91                 && hasInteriorPointers(previousCandidate->handle().cellKind())) {
     91                && mayHaveIndexingHeader(previousCandidate->handle().cellKind())) {
    9292                previousPointer = static_cast<char*>(previousCandidate->handle().cellAlign(previousPointer));
    9393                if (previousCandidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, previousPointer))
    … …  
    107107       
    108108        auto tryPointer = [&] (void* pointer) {
    109             if (candidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer))
     109            bool isLive = candidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer);
     110            if (isLive)
    110111                func(pointer, cellKind);
     112            // Only return early if we are marking a non-butterfly, since butterflies without indexed properties could point past the end of their allocation.
     113            // If we do, and there is another live butterfly immediately following the first, we will mark the latter one here but we still need to
     114            // mark the former.
     115            return isLive && !mayHaveIndexingHeader(cellKind);
    111116        };
    112117   
    113118        if (isJSCellKind(cellKind)) {
    114             if (MarkedBlock::isAtomAligned(pointer))
    115                 tryPointer(pointer);
    116             if (!hasInteriorPointers(cellKind))
    117                 return;
     119            if (LIKELY(MarkedBlock::isAtomAligned(pointer))) {
     120                if (tryPointer(pointer))
     121                    return;
     122            }
    118123        }
    119124   
    120         // A butterfly could point into the middle of an object.
     125        // We could point into the middle of an object.
    121126        char* alignedPointer = static_cast<char*>(candidate->handle().cellAlign(pointer));
    122         tryPointer(alignedPointer);
    123    
     127        if (tryPointer(alignedPointer))
     128            return;
     129
    124130        // Also, a butterfly could point at the end of an object plus sizeof(IndexingHeader). In that
    125131        // case, this is pointing to the object to the right of the one we should be marking.

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

    r262928 r263674  
    209209    switch (heapCell->cellKind()) {
    210210    case HeapCell::JSCell:
    211     case HeapCell::JSCellWithInteriorPointers: {
     211    case HeapCell::JSCellWithIndexingHeader: {
    212212        // We have ample budget to perform validation here.
    213213   

  • trunk/Source/JavaScriptCore/runtime/VM.cpp

    r263117 r263674  
    276276    , jsValueGigacageAllocator(makeUnique<GigacageAlignedMemoryAllocator>(Gigacage::JSValue))
    277277    , auxiliaryHeapCellType(makeUnique<HeapCellType>(CellAttributes(DoesNotNeedDestruction, HeapCell::Auxiliary)))
    278     , immutableButterflyHeapCellType(makeUnique<HeapCellType>(CellAttributes(DoesNotNeedDestruction, HeapCell::JSCellWithInteriorPointers)))
     278    , immutableButterflyHeapCellType(makeUnique<HeapCellType>(CellAttributes(DoesNotNeedDestruction, HeapCell::JSCellWithIndexingHeader)))
    279279    , cellHeapCellType(makeUnique<HeapCellType>(CellAttributes(DoesNotNeedDestruction, HeapCell::JSCell)))
    280280    , destructibleCellHeapCellType(makeUnique<HeapCellType>(CellAttributes(NeedsDestruction, HeapCell::JSCell)))
    … …  
    323323    , primitiveGigacageAuxiliarySpace("Primitive Gigacage Auxiliary", heap, auxiliaryHeapCellType.get(), primitiveGigacageAllocator.get()) // Hash:0x3e7cd762
    324324    , jsValueGigacageAuxiliarySpace("JSValue Gigacage Auxiliary", heap, auxiliaryHeapCellType.get(), jsValueGigacageAllocator.get()) // Hash:0x241e946
    325     , immutableButterflyJSValueGigacageAuxiliarySpace("ImmutableButterfly Gigacage JSCellWithInteriorPointers", heap, immutableButterflyHeapCellType.get(), jsValueGigacageAllocator.get()) // Hash:0x7a945300
     325    , immutableButterflyJSValueGigacageAuxiliarySpace("ImmutableButterfly Gigacage JSCellWithIndexingHeader", heap, immutableButterflyHeapCellType.get(), jsValueGigacageAllocator.get()) // Hash:0x7a945300
    326326    , cellSpace("JSCell", heap, cellHeapCellType.get(), fastMallocAllocator.get()) // Hash:0xadfb5a79
    327327    , variableSizedCellSpace("Variable Sized JSCell", heap, cellHeapCellType.get(), fastMallocAllocator.get()) // Hash:0xbcd769cc
Note: See TracChangeset for help on using the changeset viewer.