ConservativeRoots should mark any cell it finds an interior pointer to
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Jun 2020 19:07:25 +0000 (19:07 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Jun 2020 19:07:25 +0000 (19:07 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@263674 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/HeapCell.cpp
Source/JavaScriptCore/heap/HeapCell.h
Source/JavaScriptCore/heap/HeapUtil.h
Source/JavaScriptCore/heap/SlotVisitor.cpp
Source/JavaScriptCore/runtime/VM.cpp

index 6f6174c..5e0204a 100644 (file)
@@ -1,3 +1,42 @@
+2020-06-29  Keith Miller  <keith_miller@apple.com>
+
+        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):
+
 2020-06-28  Geoffrey Garen  <ggaren@apple.com>
 
         Rename initializeThreading to initialize
index 20005d6..cb4579c 100644 (file)
@@ -54,8 +54,8 @@ void printInternal(PrintStream& out, HeapCell::Kind kind)
     case HeapCell::JSCell:
         out.print("JSCell");
         return;
-    case HeapCell::JSCellWithInteriorPointers:
-        out.print("JSCellWithInteriorPointers");
+    case HeapCell::JSCellWithIndexingHeader:
+        out.print("JSCellWithIndexingHeader");
         return;
     case HeapCell::Auxiliary:
         out.print("Auxiliary");
index 0aa352a..b267366 100644 (file)
@@ -42,7 +42,7 @@ class HeapCell {
 public:
     enum Kind : int8_t {
         JSCell,
-        JSCellWithInteriorPointers,
+        JSCellWithIndexingHeader,
         Auxiliary
     };
     
@@ -93,12 +93,12 @@ public:
 
 inline bool isJSCellKind(HeapCell::Kind kind)
 {
-    return kind == HeapCell::JSCell || kind == HeapCell::JSCellWithInteriorPointers;
+    return kind == HeapCell::JSCell || kind == HeapCell::JSCellWithIndexingHeader;
 }
 
-inline bool hasInteriorPointers(HeapCell::Kind kind)
+inline bool mayHaveIndexingHeader(HeapCell::Kind kind)
 {
-    return kind == HeapCell::Auxiliary || kind == HeapCell::JSCellWithInteriorPointers;
+    return kind == HeapCell::Auxiliary || kind == HeapCell::JSCellWithIndexingHeader;
 }
 
 } // namespace JSC
index e9cdc96..d6aac3e 100644 (file)
@@ -88,7 +88,7 @@ public:
             MarkedBlock* previousCandidate = MarkedBlock::blockFor(previousPointer);
             if (!filter.ruleOut(bitwise_cast<Bits>(previousCandidate))
                 && set.contains(previousCandidate)
-                && hasInteriorPointers(previousCandidate->handle().cellKind())) {
+                && mayHaveIndexingHeader(previousCandidate->handle().cellKind())) {
                 previousPointer = static_cast<char*>(previousCandidate->handle().cellAlign(previousPointer));
                 if (previousCandidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, previousPointer))
                     func(previousPointer, previousCandidate->handle().cellKind());
@@ -106,21 +106,27 @@ public:
         HeapCell::Kind cellKind = candidate->handle().cellKind();
         
         auto tryPointer = [&] (void* pointer) {
-            if (candidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer))
+            bool isLive = candidate->handle().isLiveCell(markingVersion, newlyAllocatedVersion, isMarking, pointer);
+            if (isLive)
                 func(pointer, cellKind);
+            // Only return early if we are marking a non-butterfly, since butterflies without indexed properties could point past the end of their allocation.
+            // 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
+            // mark the former.
+            return isLive && !mayHaveIndexingHeader(cellKind);
         };
     
         if (isJSCellKind(cellKind)) {
-            if (MarkedBlock::isAtomAligned(pointer))
-                tryPointer(pointer);
-            if (!hasInteriorPointers(cellKind))
-                return;
+            if (LIKELY(MarkedBlock::isAtomAligned(pointer))) {
+                if (tryPointer(pointer))
+                    return;
+            }
         }
     
-        // A butterfly could point into the middle of an object.
+        // We could point into the middle of an object.
         char* alignedPointer = static_cast<char*>(candidate->handle().cellAlign(pointer));
-        tryPointer(alignedPointer);
-    
+        if (tryPointer(alignedPointer))
+            return;
+
         // Also, a butterfly could point at the end of an object plus sizeof(IndexingHeader). In that
         // case, this is pointing to the object to the right of the one we should be marking.
         if (candidate->candidateAtomNumber(alignedPointer) > 0
index 009d546..bfdbf44 100644 (file)
@@ -208,7 +208,7 @@ void SlotVisitor::appendJSCellOrAuxiliary(HeapCell* heapCell)
     
     switch (heapCell->cellKind()) {
     case HeapCell::JSCell:
-    case HeapCell::JSCellWithInteriorPointers: {
+    case HeapCell::JSCellWithIndexingHeader: {
         // We have ample budget to perform validation here.
     
         JSCell* jsCell = static_cast<JSCell*>(heapCell);
index 114ba97..167cf0f 100644 (file)
@@ -275,7 +275,7 @@ VM::VM(VMType vmType, HeapType heapType)
     , primitiveGigacageAllocator(makeUnique<GigacageAlignedMemoryAllocator>(Gigacage::Primitive))
     , jsValueGigacageAllocator(makeUnique<GigacageAlignedMemoryAllocator>(Gigacage::JSValue))
     , auxiliaryHeapCellType(makeUnique<HeapCellType>(CellAttributes(DoesNotNeedDestruction, HeapCell::Auxiliary)))
-    , immutableButterflyHeapCellType(makeUnique<HeapCellType>(CellAttributes(DoesNotNeedDestruction, HeapCell::JSCellWithInteriorPointers)))
+    , immutableButterflyHeapCellType(makeUnique<HeapCellType>(CellAttributes(DoesNotNeedDestruction, HeapCell::JSCellWithIndexingHeader)))
     , cellHeapCellType(makeUnique<HeapCellType>(CellAttributes(DoesNotNeedDestruction, HeapCell::JSCell)))
     , destructibleCellHeapCellType(makeUnique<HeapCellType>(CellAttributes(NeedsDestruction, HeapCell::JSCell)))
     , apiGlobalObjectHeapCellType(IsoHeapCellType::create<JSAPIGlobalObject>())
@@ -322,7 +322,7 @@ VM::VM(VMType vmType, HeapType heapType)
 #endif
     , primitiveGigacageAuxiliarySpace("Primitive Gigacage Auxiliary", heap, auxiliaryHeapCellType.get(), primitiveGigacageAllocator.get()) // Hash:0x3e7cd762
     , jsValueGigacageAuxiliarySpace("JSValue Gigacage Auxiliary", heap, auxiliaryHeapCellType.get(), jsValueGigacageAllocator.get()) // Hash:0x241e946
-    , immutableButterflyJSValueGigacageAuxiliarySpace("ImmutableButterfly Gigacage JSCellWithInteriorPointers", heap, immutableButterflyHeapCellType.get(), jsValueGigacageAllocator.get()) // Hash:0x7a945300
+    , immutableButterflyJSValueGigacageAuxiliarySpace("ImmutableButterfly Gigacage JSCellWithIndexingHeader", heap, immutableButterflyHeapCellType.get(), jsValueGigacageAllocator.get()) // Hash:0x7a945300
     , cellSpace("JSCell", heap, cellHeapCellType.get(), fastMallocAllocator.get()) // Hash:0xadfb5a79
     , variableSizedCellSpace("Variable Sized JSCell", heap, cellHeapCellType.get(), fastMallocAllocator.get()) // Hash:0xbcd769cc
     , destructibleObjectSpace("JSDestructibleObject", heap, destructibleObjectHeapCellType.get(), fastMallocAllocator.get()) // Hash:0x4f5ed7a9