Unreviewed, rolling out r206314, r206316, and r206319.

https://bugs.webkit.org/show_bug.cgi?id=162506 These changes broke various builds (Requested by ryanhaddad on #webkit). Reverted changesets: "Need a store-load fence between setting cell state and visiting the object in SlotVisitor" https://bugs.webkit.org/show_bug.cgi?id=162354 http://trac.webkit.org/changeset/206314 "Unreviewed, fix cloop." http://trac.webkit.org/changeset/206316 "Unreviewed, fix all other builds." http://trac.webkit.org/changeset/206319 Patch by Commit Queue <commit-queue@webkit.org> on 2016-09-23 Canonical link: https://commits.webkit.org/180456@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@206324 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Sep 23, 2016 · 93476ee · 93476ee
1 parent 6ddfd8f
commit 93476ee
Show file tree

Hide file tree

Showing 20 changed files with 118 additions and 104 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,24 @@
+2016-09-23  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r206314, r206316, and r206319.
+        https://bugs.webkit.org/show_bug.cgi?id=162506
+
+        These changes broke various builds (Requested by ryanhaddad on
+        #webkit).
+
+        Reverted changesets:
+
+        "Need a store-load fence between setting cell state and
+        visiting the object in SlotVisitor"
+        https://bugs.webkit.org/show_bug.cgi?id=162354
+        http://trac.webkit.org/changeset/206314
+
+        "Unreviewed, fix cloop."
+        http://trac.webkit.org/changeset/206316
+
+        "Unreviewed, fix all other builds."
+        http://trac.webkit.org/changeset/206319
+
 2016-09-23  Filip Pizlo  <fpizlo@apple.com>
 
         Unreviewed, fix all other builds.

diff --git a/Source/JavaScriptCore/assembler/AbstractMacroAssembler.h b/Source/JavaScriptCore/assembler/AbstractMacroAssembler.h
@@ -27,8 +27,6 @@
 #define AbstractMacroAssembler_h
 
 #include "AbortReason.h"
-#include "AssemblerBuffer.h"
-#include "AssemblerCommon.h"
 #include "CodeLocation.h"
 #include "MacroAssemblerCodeRef.h"
 #include "Options.h"
@@ -37,6 +35,8 @@
 #include <wtf/SharedTask.h>
 #include <wtf/WeakRandom.h>
 
+#if ENABLE(ASSEMBLER)
+
 namespace JSC {
 
 inline bool isARMv7IDIVSupported()
@@ -95,8 +95,6 @@ inline bool optimizeForX86_64()
     return isX86_64() && Options::useArchitectureSpecificOptimizations();
 }
 
-#if ENABLE(ASSEMBLER)
-
 class AllowMacroScratchRegisterUsage;
 class DisallowMacroScratchRegisterUsage;
 class LinkBuffer;
@@ -1167,8 +1165,8 @@ AbstractMacroAssembler<AssemblerType, MacroAssemblerType>::Address::indexedBy(
     return BaseIndex(base, index, scale, offset);
 }
 
-#endif // ENABLE(ASSEMBLER)
-
 } // namespace JSC
 
+#endif // ENABLE(ASSEMBLER)
+
 #endif // AbstractMacroAssembler_h
diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
@@ -11435,8 +11435,7 @@ class LowerDFGToB3 {
         LBasicBlock continuation = m_out.newBlock();
 
         m_out.branch(
-            m_out.above(loadCellState(base), m_out.constInt32(blackThreshold)),
-            usually(continuation), rarely(slowPath));
+            m_out.notZero32(loadCellState(base)), usually(continuation), rarely(slowPath));
 
         LBasicBlock lastNext = m_out.appendTo(slowPath, continuation);
 

diff --git a/Source/JavaScriptCore/heap/CellState.h b/Source/JavaScriptCore/heap/CellState.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2015 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -26,46 +26,33 @@
 #ifndef CellState_h
 #define CellState_h
 
-#include <wtf/Assertions.h>
-
 namespace JSC {
 
 enum class CellState : uint8_t {
-    // The object is black for the first time during this GC.
-    NewBlack = 0,
-
-    // The object is black for the Nth time during this full GC cycle (N > 1). An object may get to
-    // this state if it transitions from black back to grey during a concurrent GC, or because it
-    // wound up in the remembered set because of a generational barrier.
-    OldBlack = 1,
+    // The object is black as far as this GC is concerned. When not in GC, this just means that it's an
+    // old gen object. Note that we deliberately arrange OldBlack to be zero, so that the store barrier on
+    // a target object "from" is just:
+    //
+    // if (!from->cellState())
+    //     slowPath(from);
+    //
+    // There is a bunch of code in the LLInt and JITs that rely on this being the case. You'd have to
+    // change a lot of code if you ever wanted the store barrier to be anything but a non-zero check on
+    // cellState.
+    OldBlack = 0,
 
     // The object is in eden. During GC, this means that the object has not been marked yet.
-    NewWhite = 2,
-
-    // The object is grey - i.e. it will be scanned - and this is the first time in this GC that we are
-    // going to scan it. If this is an eden GC, this also means that the object is in eden.
-    NewGrey = 3,
+    NewWhite = 1,
 
     // The object is grey - i.e. it will be scanned - but it either belongs to old gen (if this is eden
     // GC) or it is grey a second time in this current GC (because a concurrent store barrier requested
     // re-greying).
-    OldGrey = 4
-};
+    OldGrey = 2,
 
-static const unsigned blackThreshold = 1; // x <= blackThreshold means x is black.
-
-inline bool isBlack(CellState cellState)
-{
-    return static_cast<unsigned>(cellState) <= blackThreshold;
-}
-
-inline CellState blacken(CellState cellState)
-{
-    if (cellState == CellState::NewGrey)
-        return CellState::NewBlack;
-    ASSERT(cellState == CellState::NewBlack || cellState == CellState::OldBlack || cellState == CellState::OldGrey);
-    return CellState::OldBlack;
-}
+    // The object is grey - i.e. it will be scanned - and this is the first time in this GC that we are
+    // going to scan it. If this is an eden GC, this also means that the object is in eden.
+    NewGrey = 3
+};
 
 } // namespace JSC
 

diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp
@@ -28,7 +28,6 @@
 #include "FullGCActivityCallback.h"
 #include "GCActivityCallback.h"
 #include "GCIncomingRefCountedSetInlines.h"
-#include "GCSegmentedArrayInlines.h"
 #include "GCTypeMap.h"
 #include "HasOwnPropertyCache.h"
 #include "HeapHelperPool.h"
@@ -915,7 +914,7 @@ void Heap::addToRememberedSet(const JSCell* cell)
 {
     ASSERT(cell);
     ASSERT(!Options::useConcurrentJIT() || !isCompilationThread());
-    ASSERT(isBlack(cell->cellState()));
+    ASSERT(cell->cellState() == CellState::OldBlack);
     // Indicate that this object is grey and that it's one of the following:
     // - A re-greyed object during a concurrent collection.
     // - An old remembered object.

diff --git a/Source/JavaScriptCore/heap/Heap.h b/Source/JavaScriptCore/heap/Heap.h
@@ -175,11 +175,11 @@ class Heap {
     // call both of these functions: Calling only one may trigger catastropic
     // memory growth.
     void reportExtraMemoryAllocated(size_t);
-    void reportExtraMemoryVisited(JSCell*, size_t);
+    void reportExtraMemoryVisited(CellState cellStateBeforeVisiting, size_t);
 
 #if ENABLE(RESOURCE_USAGE)
     // Use this API to report the subset of extra memory that lives outside this process.
-    void reportExternalMemoryVisited(JSCell*, size_t);
+    void reportExternalMemoryVisited(CellState cellStateBeforeVisiting, size_t);
     size_t externalMemorySize() { return m_externalMemorySize; }
 #endif
 

diff --git a/Source/JavaScriptCore/heap/HeapInlines.h b/Source/JavaScriptCore/heap/HeapInlines.h
@@ -125,7 +125,7 @@ inline void Heap::writeBarrier(const JSCell* from, JSCell* to)
 #if ENABLE(WRITE_BARRIER_PROFILING)
     WriteBarrierCounters::countWriteBarrier();
 #endif
-    if (!from || !isBlack(from->cellState()))
+    if (!from || from->cellState() != CellState::OldBlack)
         return;
     if (!to || to->cellState() != CellState::NewWhite)
         return;
@@ -135,7 +135,7 @@ inline void Heap::writeBarrier(const JSCell* from, JSCell* to)
 inline void Heap::writeBarrier(const JSCell* from)
 {
     ASSERT_GC_OBJECT_LOOKS_VALID(const_cast<JSCell*>(from));
-    if (!from || !isBlack(from->cellState()))
+    if (!from || from->cellState() != CellState::OldBlack)
         return;
     addToRememberedSet(from);
 }
@@ -146,10 +146,10 @@ inline void Heap::reportExtraMemoryAllocated(size_t size)
         reportExtraMemoryAllocatedSlowCase(size);
 }
 
-inline void Heap::reportExtraMemoryVisited(JSCell* cell, size_t size)
+inline void Heap::reportExtraMemoryVisited(CellState dataBeforeVisiting, size_t size)
 {
     // We don't want to double-count the extra memory that was reported in previous collections.
-    if (operationInProgress() == EdenCollection && cell->cellState() == CellState::OldBlack)
+    if (operationInProgress() == EdenCollection && dataBeforeVisiting == CellState::OldGrey)
         return;
 
     size_t* counter = &m_extraMemorySize;
@@ -162,10 +162,10 @@ inline void Heap::reportExtraMemoryVisited(JSCell* cell, size_t size)
 }
 
 #if ENABLE(RESOURCE_USAGE)
-inline void Heap::reportExternalMemoryVisited(JSCell* cell, size_t size)
+inline void Heap::reportExternalMemoryVisited(CellState dataBeforeVisiting, size_t size)
 {
     // We don't want to double-count the external memory that was reported in previous collections.
-    if (operationInProgress() == EdenCollection && cell->cellState() == CellState::OldBlack)
+    if (operationInProgress() == EdenCollection && dataBeforeVisiting == CellState::OldGrey)
         return;
 
     size_t* counter = &m_externalMemorySize;

diff --git a/Source/JavaScriptCore/heap/MarkStack.cpp b/Source/JavaScriptCore/heap/MarkStack.cpp
@@ -26,7 +26,6 @@
 #include "config.h"
 #include "MarkStack.h"
 
-#include "GCSegmentedArrayInlines.h"
 #include "JSCInlines.h"
 
 namespace JSC {

diff --git a/Source/JavaScriptCore/heap/MarkStack.h b/Source/JavaScriptCore/heap/MarkStack.h
@@ -26,7 +26,7 @@
 #ifndef MarkStack_h
 #define MarkStack_h
 
-#include "GCSegmentedArray.h"
+#include "GCSegmentedArrayInlines.h"
 
 namespace JSC {
 

diff --git a/Source/JavaScriptCore/heap/SlotVisitor.cpp b/Source/JavaScriptCore/heap/SlotVisitor.cpp
@@ -25,10 +25,9 @@
 
 #include "config.h"
 #include "SlotVisitor.h"
+#include "SlotVisitorInlines.h"
 
-#include "AbstractMacroAssembler.h"
 #include "ConservativeRoots.h"
-#include "GCSegmentedArrayInlines.h"
 #include "HeapCellInlines.h"
 #include "HeapProfiler.h"
 #include "HeapSnapshotBuilder.h"
@@ -37,7 +36,6 @@
 #include "JSObject.h"
 #include "JSString.h"
 #include "JSCInlines.h"
-#include "SlotVisitorInlines.h"
 #include "SuperSampler.h"
 #include "VM.h"
 #include <wtf/Lock.h>
@@ -298,32 +296,25 @@ ALWAYS_INLINE void SlotVisitor::visitChildren(const JSCell* cell)
 
     SetCurrentCellScope currentCellScope(*this, cell);
 
-    cell->setCellState(blacken(cell->cellState()));
-
-    // FIXME: Make this work on ARM also.
-    // https://bugs.webkit.org/show_bug.cgi?id=162461
-    if (isX86())
-        WTF::storeLoadFence();
+    m_currentObjectCellStateBeforeVisiting = cell->cellState();
+    cell->setCellState(CellState::OldBlack);
 
-    switch (cell->type()) {
-    case StringType:
+    if (isJSString(cell)) {
         JSString::visitChildren(const_cast<JSCell*>(cell), *this);
-        break;
-
-    case FinalObjectType:
+        return;
+    }
+
+    if (isJSFinalObject(cell)) {
         JSFinalObject::visitChildren(const_cast<JSCell*>(cell), *this);
-        break;
+        return;
+    }
 
-    case ArrayType:
+    if (isJSArray(cell)) {
         JSArray::visitChildren(const_cast<JSCell*>(cell), *this);
-        break;
-
-    default:
-        // FIXME: This could be so much better.
-        // https://bugs.webkit.org/show_bug.cgi?id=162462
-        cell->methodTable()->visitChildren(const_cast<JSCell*>(cell), *this);
-        break;
+        return;
     }
+
+    cell->methodTable()->visitChildren(const_cast<JSCell*>(cell), *this);
 }
 
 void SlotVisitor::donateKnownParallel()

diff --git a/Source/JavaScriptCore/heap/SlotVisitor.h b/Source/JavaScriptCore/heap/SlotVisitor.h
@@ -168,6 +168,8 @@ class SlotVisitor {
     HeapSnapshotBuilder* m_heapSnapshotBuilder { nullptr };
     JSCell* m_currentCell { nullptr };
 
+    CellState m_currentObjectCellStateBeforeVisiting { CellState::NewWhite };
+
 public:
 #if !ASSERT_DISABLED
     bool m_isCheckingForDefaultMarkViolation;

diff --git a/Source/JavaScriptCore/heap/SlotVisitorInlines.h b/Source/JavaScriptCore/heap/SlotVisitorInlines.h
@@ -106,13 +106,13 @@ inline void SlotVisitor::addUnconditionalFinalizer(UnconditionalFinalizer* uncon
 
 inline void SlotVisitor::reportExtraMemoryVisited(size_t size)
 {
-    heap()->reportExtraMemoryVisited(m_currentCell, size);
+    heap()->reportExtraMemoryVisited(m_currentObjectCellStateBeforeVisiting, size);
 }
 
 #if ENABLE(RESOURCE_USAGE)
 inline void SlotVisitor::reportExternalMemoryVisited(size_t size)
 {
-    heap()->reportExternalMemoryVisited(m_currentCell, size);
+    heap()->reportExternalMemoryVisited(m_currentObjectCellStateBeforeVisiting, size);
 }
 #endif
 

diff --git a/Source/JavaScriptCore/jit/AssemblyHelpers.h b/Source/JavaScriptCore/jit/AssemblyHelpers.h
@@ -1308,13 +1308,13 @@ class AssemblyHelpers : public MacroAssembler {
 
     Jump jumpIfIsRememberedOrInEden(GPRReg cell)
     {
-        return branch8(Above, Address(cell, JSCell::cellStateOffset()), TrustedImm32(blackThreshold));
+        return branchTest8(MacroAssembler::NonZero, MacroAssembler::Address(cell, JSCell::cellStateOffset()));
     }
 
     Jump jumpIfIsRememberedOrInEden(JSCell* cell)
     {
         uint8_t* address = reinterpret_cast<uint8_t*>(cell) + JSCell::cellStateOffset();
-        return branch8(Above, AbsoluteAddress(address), TrustedImm32(blackThreshold));
+        return branchTest8(MacroAssembler::NonZero, MacroAssembler::AbsoluteAddress(address));
     }
 
     // Emits the branch structure for typeof. The code emitted by this doesn't fall through. The

diff --git a/Source/JavaScriptCore/llint/LLIntData.cpp b/Source/JavaScriptCore/llint/LLIntData.cpp
@@ -214,7 +214,6 @@ void Data::performAssertions(VM& vm)
     STATIC_ASSERT(GetPutInfo::initializationBits == 0xffc00);
 
     STATIC_ASSERT(MarkedBlock::blockSize == 16 * 1024);
-    STATIC_ASSERT(blackThreshold == 1);
 
     ASSERT(bitwise_cast<uintptr_t>(ShadowChicken::Packet::tailMarker()) == static_cast<uintptr_t>(0x7a11));
 

diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter.asm
@@ -409,8 +409,6 @@ const NotInitialization = 2
 const MarkedBlockSize = 16 * 1024
 const MarkedBlockMask = ~(MarkedBlockSize - 1)
 
-const BlackThreshold = 1
-
 # Allocation constants
 if JSVALUE64
     const JSFinalObjectSizeClassIndex = 1
@@ -890,10 +888,9 @@ macro arrayProfile(cellAndIndexingType, profile, scratch)
     loadb JSCell::m_indexingType[cell], indexingType
 end
 
-macro skipIfIsRememberedOrInEden(cell, slowPath)
-    bba JSCell::m_cellState[cell], BlackThreshold, .done
-    slowPath()
-.done:
+macro skipIfIsRememberedOrInEden(cell, scratch1, scratch2, continuation)
+    loadb JSCell::m_cellState[cell], scratch1
+    continuation(scratch1)
 end
 
 macro notifyWrite(set, slow)