Unreviewed, rolling out r206314, r206316, and r206319.
authorryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Sep 2016 20:16:43 +0000 (20:16 +0000)
committerryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Sep 2016 20:16:43 +0000 (20:16 +0000)
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

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

20 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/assembler/AbstractMacroAssembler.h
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/heap/CellState.h
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/HeapInlines.h
Source/JavaScriptCore/heap/MarkStack.cpp
Source/JavaScriptCore/heap/MarkStack.h
Source/JavaScriptCore/heap/SlotVisitor.cpp
Source/JavaScriptCore/heap/SlotVisitor.h
Source/JavaScriptCore/heap/SlotVisitorInlines.h
Source/JavaScriptCore/jit/AssemblyHelpers.h
Source/JavaScriptCore/llint/LLIntData.cpp
Source/JavaScriptCore/llint/LowLevelInterpreter.asm
Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
Source/JavaScriptCore/runtime/JSObject.h
Source/WTF/ChangeLog
Source/WTF/wtf/Atomics.h

index 3a07213..5ab0ad8 100644 (file)
@@ -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.
index 8ac35c3..d05c913 100644 (file)
@@ -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
index a085556..af5cc39 100644 (file)
@@ -11435,8 +11435,7 @@ private:
         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);
 
index 2cb9d4b..edc716c 100644 (file)
@@ -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
 #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
 
index 81da6bd..667fdf8 100644 (file)
@@ -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.
index 65c8a36..bdb23ed 100644 (file)
@@ -175,11 +175,11 @@ public:
     // 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
 
index 89af914..0168f93 100644 (file)
@@ -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;
index c61079d..318183a 100644 (file)
@@ -26,7 +26,6 @@
 #include "config.h"
 #include "MarkStack.h"
 
-#include "GCSegmentedArrayInlines.h"
 #include "JSCInlines.h"
 
 namespace JSC {
index 1d04292..04f19c6 100644 (file)
@@ -26,7 +26,7 @@
 #ifndef MarkStack_h
 #define MarkStack_h
 
-#include "GCSegmentedArray.h"
+#include "GCSegmentedArrayInlines.h"
 
 namespace JSC {
 
index 5630116..8371721 100644 (file)
 
 #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()
index f66251b..b1e4b8b 100644 (file)
@@ -168,6 +168,8 @@ private:
     HeapSnapshotBuilder* m_heapSnapshotBuilder { nullptr };
     JSCell* m_currentCell { nullptr };
 
+    CellState m_currentObjectCellStateBeforeVisiting { CellState::NewWhite };
+
 public:
 #if !ASSERT_DISABLED
     bool m_isCheckingForDefaultMarkViolation;
index bd5e719..8ac49fa 100644 (file)
@@ -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
 
index 8668a6b..639da49 100644 (file)
@@ -1308,13 +1308,13 @@ public:
 
     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
index dfa9aa9..2ffbf09 100644 (file)
@@ -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));
 
index 10037af..997ef35 100644 (file)
@@ -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)
index 90d2c36..c3ebc62 100644 (file)
@@ -500,9 +500,9 @@ end
 macro writeBarrierOnOperand(cellOperand)
     loadisFromInstruction(cellOperand, t1)
     loadConstantOrVariablePayload(t1, CellTag, t2, .writeBarrierDone)
-    skipIfIsRememberedOrInEden(
-        t2, 
-        macro()
+    skipIfIsRememberedOrInEden(t2, t1, t3, 
+        macro(cellState)
+            btbnz cellState, .writeBarrierDone
             push cfr, PC
             # We make two extra slots because cCall2 will poke.
             subp 8, sp
@@ -511,7 +511,8 @@ macro writeBarrierOnOperand(cellOperand)
             cCall2Void(_llint_write_barrier_slow)
             addp 8, sp
             pop PC, cfr
-        end)
+        end
+    )
 .writeBarrierDone:
 end
 
@@ -531,9 +532,9 @@ macro writeBarrierOnGlobal(valueOperand, loadHelper)
 
     loadHelper(t3)
 
-    skipIfIsRememberedOrInEden(
-        t3,
-        macro()
+    skipIfIsRememberedOrInEden(t3, t1, t2,
+        macro(gcData)
+            btbnz gcData, .writeBarrierDone
             push cfr, PC
             # We make two extra slots because cCall2 will poke.
             subp 8, sp
@@ -542,7 +543,8 @@ macro writeBarrierOnGlobal(valueOperand, loadHelper)
             cCall2Void(_llint_write_barrier_slow)
             addp 8, sp
             pop PC, cfr
-        end)
+        end
+    )
 .writeBarrierDone:
 end
 
index e345652..d34a439 100644 (file)
@@ -404,15 +404,16 @@ end
 macro writeBarrierOnOperand(cellOperand)
     loadisFromInstruction(cellOperand, t1)
     loadConstantOrVariableCell(t1, t2, .writeBarrierDone)
-    skipIfIsRememberedOrInEden(
-        t2,
-        macro()
+    skipIfIsRememberedOrInEden(t2, t1, t3, 
+        macro(cellState)
+            btbnz cellState, .writeBarrierDone
             push PB, PC
             move t2, a1 # t2 can be a0 (not on 64 bits, but better safe than sorry)
             move cfr, a0
             cCall2Void(_llint_write_barrier_slow)
             pop PC, PB
-        end)
+        end
+    )
 .writeBarrierDone:
 end
 
@@ -431,9 +432,9 @@ macro writeBarrierOnGlobal(valueOperand, loadHelper)
     btpz t0, .writeBarrierDone
 
     loadHelper(t3)
-    skipIfIsRememberedOrInEden(
-        t3,
-        macro()
+    skipIfIsRememberedOrInEden(t3, t1, t2,
+        macro(gcData)
+            btbnz gcData, .writeBarrierDone
             push PB, PC
             move cfr, a0
             move t3, a1
index ed15ae1..a928760 100644 (file)
@@ -1093,7 +1093,7 @@ inline JSFinalObject* JSFinalObject::create(VM& vm, Structure* structure)
 
 inline bool isJSFinalObject(JSCell* cell)
 {
-    return cell->type() == FinalObjectType;
+    return cell->classInfo() == JSFinalObject::info();
 }
 
 inline bool isJSFinalObject(JSValue value)
index 6a08f2c..e15da85 100644 (file)
@@ -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  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         REGRESSION(r194387): Crash on github.com in IntlDateTimeFormat::resolvedOptions in C locale
index e46a4ba..c48879f 100644 (file)
@@ -175,12 +175,10 @@ inline void x86_ortop()
     // know that it is equivalent for our purposes, but it would be good to
     // investigate if that is actually better.
     MemoryBarrier();
-#elif CPU(X86_64)
+#else
     // This has acqrel semantics and is much cheaper than mfence. For exampe, in the JSC GC, using
     // mfence as a store-load fence was a 9% slow-down on Octane/splay while using this was neutral.
     asm volatile("lock; orl $0, (%%rsp)" ::: "memory");
-#else
-    asm volatile("lock; orl $0, (%%esp)" ::: "memory");
 #endif
 }