JSC::SlotVisitor should not be a hot mess
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 5 Oct 2015 17:22:20 +0000 (17:22 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 5 Oct 2015 17:22:20 +0000 (17:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=149798

Reviewed by Andreas Kling.

I had to debug JSC::SlotVisitor the other day. It was hard to follow.
Let's make it easy to follow.

* heap/Heap.cpp:
(JSC::Heap::markRoots):
(JSC::Heap::resetVisitors):
(JSC::Heap::objectCount):
(JSC::Heap::addToRememberedSet):
(JSC::Heap::collectAndSweep):
* heap/Heap.h: Deleted the string hash-consing code. It
was dead code.

Since no benchmark noticed the commit that broke this feature, perhaps
it's not worth having.

Either way, the best thing to do with dead code is to delete it.
It's still there in svn if we ever want to pick it up again.

* heap/HeapRootVisitor.h:
(JSC::HeapRootVisitor::visit):
(JSC::HeapRootVisitor::visitor): Removed the private append functions
for unsafe pointers and switched HeapRootVisitor over to the public
specially named functions for unsafe pointers.

In future, we should either remove the public specially named functions
or remove HeapRootVisitor, since they serve the same purpose. At least
for now we don't have pairs of functions on SlotVisitor that do the
exact same thing.

* heap/SlotVisitor.cpp:
(JSC::validate): Moved this static function to the top of the file.

(JSC::SlotVisitor::SlotVisitor):
(JSC::SlotVisitor::didStartMarking):
(JSC::SlotVisitor::reset): More hash cons removal.

(JSC::SlotVisitor::append):

(JSC::SlotVisitor::setMarkedAndAppendToMarkStack):
(JSC::SlotVisitor::appendToMarkStack): Renamed these functions to
distinguish them from the up-front helper functions that just do type
conversions. These are the functions that actually do stuff.

Moved these functions out of line to make it easier to set breakpoints,
and to enable code changes for debugging, like printf and synchronous
marking, without recompiling the world.

setMarkedAndAppendToMarkStack is roughly 258 bytes long (not including
function prologue and epilogue), so inlining it was probably not a
great idea in the first place.

(JSC::SlotVisitor::donateKnownParallel):
(JSC::SlotVisitor::drain):
(JSC::SlotVisitor::drainFromShared): Removed some stack probing code.
It was also dead.

(JSC::SlotVisitor::addOpaqueRoot):
(JSC::SlotVisitor::containsOpaqueRoot):
(JSC::SlotVisitor::containsOpaqueRootTriState):
(JSC::SlotVisitor::opaqueRootCount):
(JSC::SlotVisitor::mergeOpaqueRootsIfNecessary):
(JSC::SlotVisitor::mergeOpaqueRootsIfProfitable):
(JSC::SlotVisitor::donate):
(JSC::SlotVisitor::donateAndDrain):
(JSC::SlotVisitor::copyLater):
(JSC::SlotVisitor::mergeOpaqueRoots):
(JSC::SlotVisitor::harvestWeakReferences):
(JSC::SlotVisitor::finalizeUnconditionalFinalizers):
(JSC::SlotVisitor::dump): Moved more code out-of-line. These code paths
are not hot and/or not small, so we need more evidence before we inline
them. The SlotVisitor headers are included everywhere, so we should
make them include less.

Removed "internal" from all function names because it wasn't applied in
any consistent way that would mean anything.

(JSC::JSString::tryHashConsLock): Deleted.
(JSC::JSString::releaseHashConsLock): Deleted.
(JSC::JSString::shouldTryHashCons): Deleted.
(JSC::SlotVisitor::internalAppend): Deleted.
(JSC::SlotVisitor::validate): Deleted.

* heap/SlotVisitor.h:
(JSC::SlotVisitor::resetChildCount): Deleted.
(JSC::SlotVisitor::childCount): Deleted.
(JSC::SlotVisitor::incrementChildCount): Deleted. Removed this child
count thing. It was dead code.

* heap/SlotVisitorInlines.h:
(JSC::SlotVisitor::appendUnbarrieredPointer):
(JSC::SlotVisitor::appendUnbarrieredReadOnlyPointer):
(JSC::SlotVisitor::appendUnbarrieredValue):
(JSC::SlotVisitor::appendUnbarrieredReadOnlyValue): Some renaming and un-inlining.

(JSC::SlotVisitor::appendUnbarrieredWeak): Don't null check our input.
The one true place where null checking happens is our out-of-line
code. All inline functions do only type conversions.

(JSC::SlotVisitor::append):
(JSC::SlotVisitor::appendValues):
(JSC::SlotVisitor::addWeakReferenceHarvester):
(JSC::SlotVisitor::addUnconditionalFinalizer):
(JSC::SlotVisitor::reportExtraMemoryVisited): Some renaming and un-inlining.

(JSC::SlotVisitor::internalAppend): Deleted.
(JSC::SlotVisitor::unconditionallyAppend): Deleted.
(JSC::SlotVisitor::addOpaqueRoot): Deleted.
(JSC::SlotVisitor::containsOpaqueRoot): Deleted.
(JSC::SlotVisitor::containsOpaqueRootTriState): Deleted.
(JSC::SlotVisitor::opaqueRootCount): Deleted.
(JSC::SlotVisitor::mergeOpaqueRootsIfNecessary): Deleted.
(JSC::SlotVisitor::mergeOpaqueRootsIfProfitable): Deleted.
(JSC::SlotVisitor::donate): Deleted.
(JSC::SlotVisitor::donateAndDrain): Deleted.
(JSC::SlotVisitor::copyLater): Deleted.

* runtime/JSString.h:
(JSC::JSString::finishCreation):
(JSC::JSString::setIs8Bit):
(JSC::JSString::isHashConsSingleton): Deleted.
(JSC::JSString::clearHashConsSingleton): Deleted.
(JSC::JSString::setHashConsSingleton): Deleted. More hash cons removal.

* runtime/VM.cpp:
(JSC::VM::VM):
* runtime/VM.h:
(JSC::VM::currentThreadIsHoldingAPILock):
(JSC::VM::apiLock):
(JSC::VM::haveEnoughNewStringsToHashCons): Deleted.
(JSC::VM::resetNewStringsSinceLastHashCons): Deleted. More hash cons removal.

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

15 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/HeapRootVisitor.h
Source/JavaScriptCore/heap/SlotVisitor.cpp
Source/JavaScriptCore/heap/SlotVisitor.h
Source/JavaScriptCore/heap/SlotVisitorInlines.h
Source/JavaScriptCore/runtime/DirectArguments.cpp
Source/JavaScriptCore/runtime/JSMap.cpp
Source/JavaScriptCore/runtime/JSObject.cpp
Source/JavaScriptCore/runtime/JSSet.cpp
Source/JavaScriptCore/runtime/JSString.h
Source/JavaScriptCore/runtime/JSTypedArrays.cpp
Source/JavaScriptCore/runtime/VM.cpp
Source/JavaScriptCore/runtime/VM.h

index 19e8473..53c77c5 100644 (file)
@@ -1,3 +1,141 @@
+2015-10-05  Geoffrey Garen  <ggaren@apple.com>
+
+        JSC::SlotVisitor should not be a hot mess
+        https://bugs.webkit.org/show_bug.cgi?id=149798
+
+        Reviewed by Andreas Kling.
+
+        I had to debug JSC::SlotVisitor the other day. It was hard to follow.
+        Let's make it easy to follow.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::markRoots):
+        (JSC::Heap::resetVisitors):
+        (JSC::Heap::objectCount):
+        (JSC::Heap::addToRememberedSet):
+        (JSC::Heap::collectAndSweep):
+        * heap/Heap.h: Deleted the string hash-consing code. It
+        was dead code.
+
+        Since no benchmark noticed the commit that broke this feature, perhaps
+        it's not worth having.
+
+        Either way, the best thing to do with dead code is to delete it.
+        It's still there in svn if we ever want to pick it up again.
+
+        * heap/HeapRootVisitor.h:
+        (JSC::HeapRootVisitor::visit):
+        (JSC::HeapRootVisitor::visitor): Removed the private append functions
+        for unsafe pointers and switched HeapRootVisitor over to the public
+        specially named functions for unsafe pointers.
+
+        In future, we should either remove the public specially named functions
+        or remove HeapRootVisitor, since they serve the same purpose. At least
+        for now we don't have pairs of functions on SlotVisitor that do the
+        exact same thing.
+
+        * heap/SlotVisitor.cpp:
+        (JSC::validate): Moved this static function to the top of the file.
+
+        (JSC::SlotVisitor::SlotVisitor):
+        (JSC::SlotVisitor::didStartMarking):
+        (JSC::SlotVisitor::reset): More hash cons removal.
+
+        (JSC::SlotVisitor::append):
+
+        (JSC::SlotVisitor::setMarkedAndAppendToMarkStack):
+        (JSC::SlotVisitor::appendToMarkStack): Renamed these functions to 
+        distinguish them from the up-front helper functions that just do type
+        conversions. These are the functions that actually do stuff.
+
+        Moved these functions out of line to make it easier to set breakpoints,
+        and to enable code changes for debugging, like printf and synchronous
+        marking, without recompiling the world.
+
+        setMarkedAndAppendToMarkStack is roughly 258 bytes long (not including
+        function prologue and epilogue), so inlining it was probably not a
+        great idea in the first place.
+
+        (JSC::SlotVisitor::donateKnownParallel):
+        (JSC::SlotVisitor::drain):
+        (JSC::SlotVisitor::drainFromShared): Removed some stack probing code.
+        It was also dead.
+
+        (JSC::SlotVisitor::addOpaqueRoot):
+        (JSC::SlotVisitor::containsOpaqueRoot):
+        (JSC::SlotVisitor::containsOpaqueRootTriState):
+        (JSC::SlotVisitor::opaqueRootCount):
+        (JSC::SlotVisitor::mergeOpaqueRootsIfNecessary):
+        (JSC::SlotVisitor::mergeOpaqueRootsIfProfitable):
+        (JSC::SlotVisitor::donate):
+        (JSC::SlotVisitor::donateAndDrain):
+        (JSC::SlotVisitor::copyLater):
+        (JSC::SlotVisitor::mergeOpaqueRoots):
+        (JSC::SlotVisitor::harvestWeakReferences):
+        (JSC::SlotVisitor::finalizeUnconditionalFinalizers):
+        (JSC::SlotVisitor::dump): Moved more code out-of-line. These code paths
+        are not hot and/or not small, so we need more evidence before we inline
+        them. The SlotVisitor headers are included everywhere, so we should
+        make them include less.
+
+        Removed "internal" from all function names because it wasn't applied in
+        any consistent way that would mean anything.
+
+        (JSC::JSString::tryHashConsLock): Deleted.
+        (JSC::JSString::releaseHashConsLock): Deleted.
+        (JSC::JSString::shouldTryHashCons): Deleted.
+        (JSC::SlotVisitor::internalAppend): Deleted.
+        (JSC::SlotVisitor::validate): Deleted.
+
+        * heap/SlotVisitor.h:
+        (JSC::SlotVisitor::resetChildCount): Deleted.
+        (JSC::SlotVisitor::childCount): Deleted.
+        (JSC::SlotVisitor::incrementChildCount): Deleted. Removed this child
+        count thing. It was dead code.
+
+        * heap/SlotVisitorInlines.h:
+        (JSC::SlotVisitor::appendUnbarrieredPointer):
+        (JSC::SlotVisitor::appendUnbarrieredReadOnlyPointer):
+        (JSC::SlotVisitor::appendUnbarrieredValue):
+        (JSC::SlotVisitor::appendUnbarrieredReadOnlyValue): Some renaming and un-inlining.
+
+        (JSC::SlotVisitor::appendUnbarrieredWeak): Don't null check our input.
+        The one true place where null checking happens is our out-of-line
+        code. All inline functions do only type conversions.
+
+        (JSC::SlotVisitor::append):
+        (JSC::SlotVisitor::appendValues):
+        (JSC::SlotVisitor::addWeakReferenceHarvester):
+        (JSC::SlotVisitor::addUnconditionalFinalizer):
+        (JSC::SlotVisitor::reportExtraMemoryVisited): Some renaming and un-inlining.
+
+        (JSC::SlotVisitor::internalAppend): Deleted.
+        (JSC::SlotVisitor::unconditionallyAppend): Deleted.
+        (JSC::SlotVisitor::addOpaqueRoot): Deleted.
+        (JSC::SlotVisitor::containsOpaqueRoot): Deleted.
+        (JSC::SlotVisitor::containsOpaqueRootTriState): Deleted.
+        (JSC::SlotVisitor::opaqueRootCount): Deleted.
+        (JSC::SlotVisitor::mergeOpaqueRootsIfNecessary): Deleted.
+        (JSC::SlotVisitor::mergeOpaqueRootsIfProfitable): Deleted.
+        (JSC::SlotVisitor::donate): Deleted.
+        (JSC::SlotVisitor::donateAndDrain): Deleted.
+        (JSC::SlotVisitor::copyLater): Deleted.
+
+        * runtime/JSString.h:
+        (JSC::JSString::finishCreation):
+        (JSC::JSString::setIs8Bit):
+        (JSC::JSString::isHashConsSingleton): Deleted.
+        (JSC::JSString::clearHashConsSingleton): Deleted.
+        (JSC::JSString::setHashConsSingleton): Deleted. More hash cons removal.
+
+        * runtime/VM.cpp:
+        (JSC::VM::VM):
+        * runtime/VM.h:
+        (JSC::VM::currentThreadIsHoldingAPILock):
+        (JSC::VM::apiLock):
+        (JSC::VM::haveEnoughNewStringsToHashCons): Deleted.
+        (JSC::VM::resetNewStringsSinceLastHashCons): Deleted. More hash cons removal.
+
 2015-10-04  Filip Pizlo  <fpizlo@apple.com>
 
         Inline cache repatching should be throttled if it happens a lot
index fbfdd40..fe107c8 100644 (file)
@@ -543,8 +543,6 @@ void Heap::markRoots(double gcStartTime, void* stackOrigin, void* stackTop, Mach
     if (m_operationInProgress == FullCollection)
         m_opaqueRoots.clear();
 
-    m_shouldHashCons = m_vm->haveEnoughNewStringsToHashCons();
-
     m_parallelMarkersShouldExit = false;
 
     m_helperClient.setFunction(
@@ -895,10 +893,6 @@ void Heap::resetVisitors()
 
     ASSERT(m_sharedMarkStack.isEmpty());
     m_weakReferenceHarvesters.removeAll();
-    if (m_shouldHashCons) {
-        m_vm->resetNewStringsSinceLastHashCons();
-        m_shouldHashCons = false;
-    }
 }
 
 size_t Heap::objectCount()
@@ -1017,7 +1011,7 @@ void Heap::addToRememberedSet(const JSCell* cell)
     if (isRemembered(cell))
         return;
     const_cast<JSCell*>(cell)->setRemembered(true);
-    m_slotVisitor.unconditionallyAppend(const_cast<JSCell*>(cell));
+    m_slotVisitor.appendToMarkStack(const_cast<JSCell*>(cell));
 }
 
 void Heap::collectAndSweep(HeapOperation collectionType)
index f1cc2cd..cb269fb 100644 (file)
@@ -423,8 +423,6 @@ private:
 
     HashMap<void*, std::function<void()>> m_weakGCMaps;
 
-    bool m_shouldHashCons { false };
-
     Lock m_markingMutex;
     Condition m_markingConditionVariable;
     MarkStackArray m_sharedMarkStack;
index 5b11a5e..cac0b37 100644 (file)
@@ -58,22 +58,23 @@ namespace JSC {
 
     inline void HeapRootVisitor::visit(JSValue* slot)
     {
-        m_visitor.append(slot);
+        m_visitor.appendUnbarrieredValue(slot);
     }
 
     inline void HeapRootVisitor::visit(JSValue* slot, size_t count)
     {
-        m_visitor.append(slot, count);
+        for (size_t i = 0; i < count; ++i)
+            m_visitor.appendUnbarrieredValue(&slot[i]);
     }
 
     inline void HeapRootVisitor::visit(JSString** slot)
     {
-        m_visitor.append(reinterpret_cast<JSCell**>(slot));
+        m_visitor.appendUnbarrieredPointer(slot);
     }
 
     inline void HeapRootVisitor::visit(JSCell** slot)
     {
-        m_visitor.append(slot);
+        m_visitor.appendUnbarrieredPointer(slot);
     }
 
     inline SlotVisitor& HeapRootVisitor::visitor()
index ebad965..da3454b 100644 (file)
@@ -28,6 +28,7 @@
 #include "SlotVisitorInlines.h"
 
 #include "ConservativeRoots.h"
+#include "CopiedBlockInlines.h"
 #include "CopiedSpace.h"
 #include "CopiedSpaceInlines.h"
 #include "JSArray.h"
 #include "JSString.h"
 #include "JSCInlines.h"
 #include <wtf/Lock.h>
-#include <wtf/StackStats.h>
 
 namespace JSC {
 
+#if ENABLE(GC_VALIDATION)
+static void validate(JSCell* cell)
+{
+    RELEASE_ASSERT(cell);
+
+    if (!cell->structure()) {
+        dataLogF("cell at %p has a null structure\n" , cell);
+        CRASH();
+    }
+
+    // Both the cell's structure, and the cell's structure's structure should be the Structure Structure.
+    // I hate this sentence.
+    if (cell->structure()->structure()->JSCell::classInfo() != cell->structure()->JSCell::classInfo()) {
+        const char* parentClassName = 0;
+        const char* ourClassName = 0;
+        if (cell->structure()->structure() && cell->structure()->structure()->JSCell::classInfo())
+            parentClassName = cell->structure()->structure()->JSCell::classInfo()->className;
+        if (cell->structure()->JSCell::classInfo())
+            ourClassName = cell->structure()->JSCell::classInfo()->className;
+        dataLogF("parent structure (%p <%s>) of cell at %p doesn't match cell's structure (%p <%s>)\n",
+            cell->structure()->structure(), parentClassName, cell, cell->structure(), ourClassName);
+        CRASH();
+    }
+
+    // Make sure we can walk the ClassInfo chain
+    const ClassInfo* info = cell->classInfo();
+    do { } while ((info = info->parentClass));
+}
+#endif
+
 SlotVisitor::SlotVisitor(Heap& heap)
     : m_stack()
     , m_bytesVisited(0)
@@ -48,7 +78,6 @@ SlotVisitor::SlotVisitor(Heap& heap)
     , m_visitCount(0)
     , m_isInParallelMode(false)
     , m_heap(heap)
-    , m_shouldHashCons(false)
 #if !ASSERT_DISABLED
     , m_isCheckingForDefaultMarkViolation(false)
     , m_isDraining(false)
@@ -65,8 +94,6 @@ void SlotVisitor::didStartMarking()
 {
     if (heap()->operationInProgress() == FullCollection)
         ASSERT(m_opaqueRoots.isEmpty()); // Should have merged by now.
-
-    m_shouldHashCons = m_heap.m_shouldHashCons;
 }
 
 void SlotVisitor::reset()
@@ -75,10 +102,6 @@ void SlotVisitor::reset()
     m_bytesCopied = 0;
     m_visitCount = 0;
     ASSERT(m_stack.isEmpty());
-    if (m_shouldHashCons) {
-        m_uniqueStrings.clear();
-        m_shouldHashCons = false;
-    }
 }
 
 void SlotVisitor::clearMarkStack()
@@ -88,17 +111,50 @@ void SlotVisitor::clearMarkStack()
 
 void SlotVisitor::append(ConservativeRoots& conservativeRoots)
 {
-    StackStats::probe();
     JSCell** roots = conservativeRoots.roots();
     size_t size = conservativeRoots.size();
     for (size_t i = 0; i < size; ++i)
-        internalAppend(0, roots[i]);
+        append(roots[i]);
 }
 
-ALWAYS_INLINE static void visitChildren(SlotVisitor& visitor, const JSCell* cell)
+void SlotVisitor::append(JSValue value)
 {
-    StackStats::probe();
+    if (!value || !value.isCell())
+        return;
+    setMarkedAndAppendToMarkStack(value.asCell());
+}
+
+void SlotVisitor::setMarkedAndAppendToMarkStack(JSCell* cell)
+{
+    ASSERT(!m_isCheckingForDefaultMarkViolation);
+    if (!cell)
+        return;
+
+#if ENABLE(GC_VALIDATION)
+    validate(cell);
+#endif
+
+    if (Heap::testAndSetMarked(cell) || !cell->structure()) {
+        ASSERT(cell->structure());
+        return;
+    }
 
+    cell->setMarked();
+    appendToMarkStack(cell);
+}
+
+void SlotVisitor::appendToMarkStack(JSCell* cell)
+{
+    ASSERT(Heap::isMarked(cell));
+    ASSERT(!cell->isZapped());
+
+    m_visitCount++;
+    m_bytesVisited += MarkedBlock::blockFor(cell)->cellSize();
+    m_stack.append(cell);
+}
+
+ALWAYS_INLINE static void visitChildren(SlotVisitor& visitor, const JSCell* cell)
+{
     ASSERT(Heap::isMarked(cell));
     
     if (isJSString(cell)) {
@@ -121,7 +177,6 @@ ALWAYS_INLINE static void visitChildren(SlotVisitor& visitor, const JSCell* cell
 
 void SlotVisitor::donateKnownParallel()
 {
-    StackStats::probe();
     // NOTE: Because we re-try often, we can afford to be conservative, and
     // assume that donating is not profitable.
 
@@ -148,7 +203,6 @@ void SlotVisitor::donateKnownParallel()
 
 void SlotVisitor::drain()
 {
-    StackStats::probe();
     ASSERT(m_isInParallelMode);
    
     while (!m_stack.isEmpty()) {
@@ -163,7 +217,6 @@ void SlotVisitor::drain()
 
 void SlotVisitor::drainFromShared(SharedDrainMode sharedDrainMode)
 {
-    StackStats::probe();
     ASSERT(m_isInParallelMode);
     
     ASSERT(Options::numberOfGCMarkers());
@@ -228,133 +281,118 @@ void SlotVisitor::drainFromShared(SharedDrainMode sharedDrainMode)
     }
 }
 
-void SlotVisitor::mergeOpaqueRoots()
+void SlotVisitor::addOpaqueRoot(void* root)
 {
-    StackStats::probe();
-    ASSERT(!m_opaqueRoots.isEmpty()); // Should only be called when opaque roots are non-empty.
-    {
-        std::lock_guard<Lock> lock(m_heap.m_opaqueRootsMutex);
-        for (auto* root : m_opaqueRoots)
-            m_heap.m_opaqueRoots.add(root);
+    if (Options::numberOfGCMarkers() == 1) {
+        // Put directly into the shared HashSet.
+        m_heap.m_opaqueRoots.add(root);
+        return;
     }
-    m_opaqueRoots.clear();
+    // Put into the local set, but merge with the shared one every once in
+    // a while to make sure that the local sets don't grow too large.
+    mergeOpaqueRootsIfProfitable();
+    m_opaqueRoots.add(root);
 }
 
-ALWAYS_INLINE bool JSString::tryHashConsLock()
+bool SlotVisitor::containsOpaqueRoot(void* root) const
 {
-    unsigned currentFlags = m_flags;
-
-    if (currentFlags & HashConsLock)
-        return false;
-
-    unsigned newFlags = currentFlags | HashConsLock;
-
-    if (!WTF::weakCompareAndSwap(&m_flags, currentFlags, newFlags))
-        return false;
-
-    WTF::memoryBarrierAfterLock();
-    return true;
+    ASSERT(!m_isInParallelMode);
+    ASSERT(m_opaqueRoots.isEmpty());
+    return m_heap.m_opaqueRoots.contains(root);
 }
 
-ALWAYS_INLINE void JSString::releaseHashConsLock()
+TriState SlotVisitor::containsOpaqueRootTriState(void* root) const
 {
-    WTF::memoryBarrierBeforeUnlock();
-    m_flags &= ~HashConsLock;
+    if (m_opaqueRoots.contains(root))
+        return TrueTriState;
+    std::lock_guard<Lock> lock(m_heap.m_opaqueRootsMutex);
+    if (m_heap.m_opaqueRoots.contains(root))
+        return TrueTriState;
+    return MixedTriState;
 }
 
-ALWAYS_INLINE bool JSString::shouldTryHashCons()
+int SlotVisitor::opaqueRootCount()
 {
-    return ((length() > 1) && !isRope() && !isHashConsSingleton());
+    ASSERT(!m_isInParallelMode);
+    ASSERT(m_opaqueRoots.isEmpty());
+    return m_heap.m_opaqueRoots.size();
 }
 
-ALWAYS_INLINE void SlotVisitor::internalAppend(void* from, JSValue* slot)
+void SlotVisitor::mergeOpaqueRootsIfNecessary()
 {
-    // This internalAppend is only intended for visits to object and array backing stores.
-    // as it can change the JSValue pointed to be the argument when the original JSValue
-    // is a string that contains the same contents as another string.
-
-    StackStats::probe();
-    ASSERT(slot);
-    JSValue value = *slot;
-    ASSERT(value);
-    if (!value.isCell())
+    if (m_opaqueRoots.isEmpty())
         return;
-
-    JSCell* cell = value.asCell();
-    if (!cell)
+    mergeOpaqueRoots();
+}
+    
+void SlotVisitor::mergeOpaqueRootsIfProfitable()
+{
+    if (static_cast<unsigned>(m_opaqueRoots.size()) < Options::opaqueRootMergeThreshold())
+        return;
+    mergeOpaqueRoots();
+}
+    
+void SlotVisitor::donate()
+{
+    ASSERT(m_isInParallelMode);
+    if (Options::numberOfGCMarkers() == 1)
         return;
+    
+    donateKnownParallel();
+}
 
-    validate(cell);
+void SlotVisitor::donateAndDrain()
+{
+    donate();
+    drain();
+}
 
-    if (m_shouldHashCons && cell->isString()) {
-        JSString* string = jsCast<JSString*>(cell);
-        if (string->shouldTryHashCons() && string->tryHashConsLock()) {
-            UniqueStringMap::AddResult addResult = m_uniqueStrings.add(string->string().impl(), value);
-            if (addResult.isNewEntry)
-                string->setHashConsSingleton();
-            else {
-                JSValue existingJSValue = addResult.iterator->value;
-                if (value != existingJSValue)
-                    jsCast<JSString*>(existingJSValue.asCell())->clearHashConsSingleton();
-                *slot = existingJSValue;
-                string->releaseHashConsLock();
-                return;
-            }
-            string->releaseHashConsLock();
-        }
+void SlotVisitor::copyLater(JSCell* owner, CopyToken token, void* ptr, size_t bytes)
+{
+    ASSERT(bytes);
+    CopiedBlock* block = CopiedSpace::blockFor(ptr);
+    if (block->isOversize()) {
+        ASSERT(bytes <= block->size());
+        // FIXME: We should be able to shrink the allocation if bytes went below the block size.
+        // For now, we just make sure that our accounting of how much memory we are actually using
+        // is correct.
+        // https://bugs.webkit.org/show_bug.cgi?id=144749
+        bytes = block->size();
+        m_heap.m_storageSpace.pin(block);
     }
 
-    internalAppend(from, cell);
+    ASSERT(heap()->m_storageSpace.contains(block));
+
+    LockHolder locker(&block->workListLock());
+    if (heap()->operationInProgress() == FullCollection || block->shouldReportLiveBytes(locker, owner)) {
+        m_bytesCopied += bytes;
+        block->reportLiveBytes(locker, owner, token, bytes);
+    }
+}
+    
+void SlotVisitor::mergeOpaqueRoots()
+{
+    ASSERT(!m_opaqueRoots.isEmpty()); // Should only be called when opaque roots are non-empty.
+    {
+        std::lock_guard<Lock> lock(m_heap.m_opaqueRootsMutex);
+        for (auto* root : m_opaqueRoots)
+            m_heap.m_opaqueRoots.add(root);
+    }
+    m_opaqueRoots.clear();
 }
 
 void SlotVisitor::harvestWeakReferences()
 {
-    StackStats::probe();
     for (WeakReferenceHarvester* current = m_heap.m_weakReferenceHarvesters.head(); current; current = current->next())
         current->visitWeakReferences(*this);
 }
 
 void SlotVisitor::finalizeUnconditionalFinalizers()
 {
-    StackStats::probe();
     while (m_heap.m_unconditionalFinalizers.hasNext())
         m_heap.m_unconditionalFinalizers.removeNext()->finalizeUnconditionally();
 }
 
-#if ENABLE(GC_VALIDATION)
-void SlotVisitor::validate(JSCell* cell)
-{
-    RELEASE_ASSERT(cell);
-
-    if (!cell->structure()) {
-        dataLogF("cell at %p has a null structure\n" , cell);
-        CRASH();
-    }
-
-    // Both the cell's structure, and the cell's structure's structure should be the Structure Structure.
-    // I hate this sentence.
-    if (cell->structure()->structure()->JSCell::classInfo() != cell->structure()->JSCell::classInfo()) {
-        const char* parentClassName = 0;
-        const char* ourClassName = 0;
-        if (cell->structure()->structure() && cell->structure()->structure()->JSCell::classInfo())
-            parentClassName = cell->structure()->structure()->JSCell::classInfo()->className;
-        if (cell->structure()->JSCell::classInfo())
-            ourClassName = cell->structure()->JSCell::classInfo()->className;
-        dataLogF("parent structure (%p <%s>) of cell at %p doesn't match cell's structure (%p <%s>)\n",
-                cell->structure()->structure(), parentClassName, cell, cell->structure(), ourClassName);
-        CRASH();
-    }
-
-    // Make sure we can walk the ClassInfo chain
-    const ClassInfo* info = cell->classInfo();
-    do { } while ((info = info->parentClass));
-}
-#else
-void SlotVisitor::validate(JSCell*)
-{
-}
-#endif
-
 void SlotVisitor::dump(PrintStream&) const
 {
     for (const JSCell* cell : markStack())
index 580396f..c063a1c 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, 2012, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2012, 2013, 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
@@ -31,9 +31,6 @@
 #include "MarkStack.h"
 #include "OpaqueRootSet.h"
 
-#include <wtf/HashSet.h>
-#include <wtf/text/StringHash.h>
-
 namespace JSC {
 
 class ConservativeRoots;
@@ -46,8 +43,11 @@ class WeakReferenceHarvester;
 template<typename T> class WriteBarrierBase;
 
 class SlotVisitor {
-    WTF_MAKE_NONCOPYABLE(SlotVisitor); WTF_MAKE_FAST_ALLOCATED;
+    WTF_MAKE_NONCOPYABLE(SlotVisitor);
+    WTF_MAKE_FAST_ALLOCATED;
+
     friend class HeapRootVisitor; // Allowed to mark a JSValue* or JSCell** directly.
+    friend class Heap;
 
 public:
     SlotVisitor(Heap&);
@@ -75,10 +75,9 @@ public:
     template<typename T>
     void appendUnbarrieredReadOnlyPointer(T*);
     void appendUnbarrieredReadOnlyValue(JSValue);
-    void unconditionallyAppend(JSCell*);
     
-    void addOpaqueRoot(void*);
-    bool containsOpaqueRoot(void*) const;
+    JS_EXPORT_PRIVATE void addOpaqueRoot(void*);
+    JS_EXPORT_PRIVATE bool containsOpaqueRoot(void*) const;
     TriState containsOpaqueRootTriState(void*) const;
     int opaqueRootCount();
 
@@ -109,24 +108,15 @@ public:
     void addWeakReferenceHarvester(WeakReferenceHarvester*);
     void addUnconditionalFinalizer(UnconditionalFinalizer*);
 
-    inline void resetChildCount() { m_logChildCount = 0; }
-    inline unsigned childCount() { return m_logChildCount; }
-    inline void incrementChildCount() { m_logChildCount++; }
-
     void dump(PrintStream&) const;
 
 private:
     friend class ParallelModeEnabler;
     
-    JS_EXPORT_PRIVATE static void validate(JSCell*);
+    JS_EXPORT_PRIVATE void append(JSValue); // This is private to encourage clients to use WriteBarrier<T>.
 
-    void append(JSValue*);
-    void append(JSValue*, size_t count);
-    void append(JSCell**);
-    
-    void internalAppend(void* from, JSCell*);
-    void internalAppend(void* from, JSValue);
-    void internalAppend(void* from, JSValue*);
+    JS_EXPORT_PRIVATE void setMarkedAndAppendToMarkStack(JSCell*);
+    void appendToMarkStack(JSCell*);
     
     JS_EXPORT_PRIVATE void mergeOpaqueRoots();
     void mergeOpaqueRootsIfNecessary();
@@ -144,12 +134,6 @@ private:
     
     Heap& m_heap;
 
-    bool m_shouldHashCons; // Local per-thread copy of shared flag for performance reasons
-    typedef HashMap<StringImpl*, JSValue> UniqueStringMap;
-    UniqueStringMap m_uniqueStrings;
-
-    unsigned m_logChildCount;
-
 public:
 #if !ASSERT_DISABLED
     bool m_isCheckingForDefaultMarkViolation;
index a4afc7d..dab8daf 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2013, 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 SlotVisitorInlines_h
 #define SlotVisitorInlines_h
 
-#include "CopiedBlockInlines.h"
-#include "CopiedSpaceInlines.h"
-#include "Options.h"
 #include "SlotVisitor.h"
 #include "Weak.h"
 #include "WeakInlines.h"
 
 namespace JSC {
 
-ALWAYS_INLINE void SlotVisitor::append(JSValue* slot, size_t count)
-{
-    for (size_t i = 0; i < count; ++i) {
-        JSValue& value = slot[i];
-        internalAppend(&value, value);
-    }
-}
-
 template<typename T>
 inline void SlotVisitor::appendUnbarrieredPointer(T** slot)
 {
     ASSERT(slot);
-    JSCell* cell = *slot;
-    internalAppend(slot, cell);
+    append(*slot);
 }
 
 template<typename T>
 inline void SlotVisitor::appendUnbarrieredReadOnlyPointer(T* cell)
 {
-    internalAppend(0, cell);
-}
-
-ALWAYS_INLINE void SlotVisitor::append(JSValue* slot)
-{
-    ASSERT(slot);
-    internalAppend(slot, *slot);
+    append(cell);
 }
 
-ALWAYS_INLINE void SlotVisitor::appendUnbarrieredValue(JSValue* slot)
+inline void SlotVisitor::appendUnbarrieredValue(JSValue* slot)
 {
     ASSERT(slot);
-    internalAppend(slot, *slot);
-}
-
-ALWAYS_INLINE void SlotVisitor::appendUnbarrieredReadOnlyValue(JSValue value)
-{
-    internalAppend(0, value);
+    append(*slot);
 }
 
-ALWAYS_INLINE void SlotVisitor::append(JSCell** slot)
+inline void SlotVisitor::appendUnbarrieredReadOnlyValue(JSValue value)
 {
-    ASSERT(slot);
-    internalAppend(slot, *slot);
+    append(value);
 }
 
 template<typename T>
-ALWAYS_INLINE void SlotVisitor::appendUnbarrieredWeak(Weak<T>* weak)
+inline void SlotVisitor::appendUnbarrieredWeak(Weak<T>* weak)
 {
     ASSERT(weak);
-    if (weak->get())
-        internalAppend(0, weak->get());
-}
-
-ALWAYS_INLINE void SlotVisitor::internalAppend(void* from, JSValue value)
-{
-    if (!value || !value.isCell())
-        return;
-    internalAppend(from, value.asCell());
-}
-
-ALWAYS_INLINE void SlotVisitor::internalAppend(void* from, JSCell* cell)
-{
-    ASSERT(!m_isCheckingForDefaultMarkViolation);
-    if (!cell)
-        return;
-#if ENABLE(ALLOCATION_LOGGING)
-    dataLogF("JSC GC noticing reference from %p to %p.\n", from, cell);
-#else
-    UNUSED_PARAM(from);
-#endif
-#if ENABLE(GC_VALIDATION)
-    validate(cell);
-#endif
-    if (Heap::testAndSetMarked(cell) || !cell->structure()) {
-        ASSERT(cell->structure());
-        return;
-    }
-
-    cell->setMarked();
-    m_bytesVisited += MarkedBlock::blockFor(cell)->cellSize();
-        
-    unconditionallyAppend(cell);
+    append(weak->get());
 }
 
-ALWAYS_INLINE void SlotVisitor::unconditionallyAppend(JSCell* cell)
-{
-    ASSERT(Heap::isMarked(cell));
-    m_visitCount++;
-        
-    // Should never attempt to mark something that is zapped.
-    ASSERT(!cell->isZapped());
-        
-    m_stack.append(cell);
-}
-
-template<typename T> inline void SlotVisitor::append(WriteBarrierBase<T>* slot)
+template<typename T>
+inline void SlotVisitor::append(WriteBarrierBase<T>* slot)
 {
-    internalAppend(slot, *slot->slot());
+    append(slot->get());
 }
 
-template<typename Iterator> inline void SlotVisitor::append(Iterator begin, Iterator end)
+template<typename Iterator>
+inline void SlotVisitor::append(Iterator begin, Iterator end)
 {
     for (auto it = begin; it != end; ++it)
         append(&*it);
 }
 
-ALWAYS_INLINE void SlotVisitor::appendValues(WriteBarrierBase<Unknown>* barriers, size_t count)
+inline void SlotVisitor::appendValues(WriteBarrierBase<Unknown>* barriers, size_t count)
 {
-    append(barriers->slot(), count);
+    for (size_t i = 0; i < count; ++i)
+        append(&barriers[i]);
 }
 
 inline void SlotVisitor::addWeakReferenceHarvester(WeakReferenceHarvester* weakReferenceHarvester)
@@ -156,95 +92,6 @@ inline void SlotVisitor::addUnconditionalFinalizer(UnconditionalFinalizer* uncon
     m_heap.m_unconditionalFinalizers.addThreadSafe(unconditionalFinalizer);
 }
 
-inline void SlotVisitor::addOpaqueRoot(void* root)
-{
-    if (Options::numberOfGCMarkers() == 1) {
-        // Put directly into the shared HashSet.
-        m_heap.m_opaqueRoots.add(root);
-        return;
-    }
-    // Put into the local set, but merge with the shared one every once in
-    // a while to make sure that the local sets don't grow too large.
-    mergeOpaqueRootsIfProfitable();
-    m_opaqueRoots.add(root);
-}
-
-inline bool SlotVisitor::containsOpaqueRoot(void* root) const
-{
-    ASSERT(!m_isInParallelMode);
-    ASSERT(m_opaqueRoots.isEmpty());
-    return m_heap.m_opaqueRoots.contains(root);
-}
-
-inline TriState SlotVisitor::containsOpaqueRootTriState(void* root) const
-{
-    if (m_opaqueRoots.contains(root))
-        return TrueTriState;
-    std::lock_guard<Lock> lock(m_heap.m_opaqueRootsMutex);
-    if (m_heap.m_opaqueRoots.contains(root))
-        return TrueTriState;
-    return MixedTriState;
-}
-
-inline int SlotVisitor::opaqueRootCount()
-{
-    ASSERT(!m_isInParallelMode);
-    ASSERT(m_opaqueRoots.isEmpty());
-    return m_heap.m_opaqueRoots.size();
-}
-
-inline void SlotVisitor::mergeOpaqueRootsIfNecessary()
-{
-    if (m_opaqueRoots.isEmpty())
-        return;
-    mergeOpaqueRoots();
-}
-    
-inline void SlotVisitor::mergeOpaqueRootsIfProfitable()
-{
-    if (static_cast<unsigned>(m_opaqueRoots.size()) < Options::opaqueRootMergeThreshold())
-        return;
-    mergeOpaqueRoots();
-}
-    
-inline void SlotVisitor::donate()
-{
-    ASSERT(m_isInParallelMode);
-    if (Options::numberOfGCMarkers() == 1)
-        return;
-    
-    donateKnownParallel();
-}
-
-inline void SlotVisitor::donateAndDrain()
-{
-    donate();
-    drain();
-}
-
-inline void SlotVisitor::copyLater(JSCell* owner, CopyToken token, void* ptr, size_t bytes)
-{
-    ASSERT(bytes);
-    CopiedBlock* block = CopiedSpace::blockFor(ptr);
-    if (block->isOversize()) {
-        ASSERT(bytes <= block->size());
-        // FIXME: We should be able to shrink the allocation if bytes went below the block size.
-        // For now, we just make sure that our accounting of how much memory we are actually using
-        // is correct.
-        // https://bugs.webkit.org/show_bug.cgi?id=144749
-        bytes = block->size();
-        m_heap.m_storageSpace.pin(block);
-    }
-
-    ASSERT(heap()->m_storageSpace.contains(block));
-
-    LockHolder locker(&block->workListLock());
-    if (heap()->operationInProgress() == FullCollection || block->shouldReportLiveBytes(locker, owner)) {
-        m_bytesCopied += bytes;
-        block->reportLiveBytes(locker, owner, token, bytes);
-    }
-}
-    
 inline void SlotVisitor::reportExtraMemoryVisited(JSCell* owner, size_t size)
 {
     heap()->reportExtraMemoryVisited(owner, size);
index 240d51d..2cf688c 100644 (file)
@@ -27,6 +27,7 @@
 #include "DirectArguments.h"
 
 #include "CodeBlock.h"
+#include "CopiedBlockInlines.h"
 #include "CopyVisitorInlines.h"
 #include "GenericArgumentsInlines.h"
 #include "JSCInlines.h"
index 352648b..44d0f39 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "JSMap.h"
 
+#include "CopiedBlockInlines.h"
 #include "JSCJSValueInlines.h"
 #include "JSMapIterator.h"
 #include "MapDataInlines.h"
index fc4b9a3..c95d336 100644 (file)
@@ -25,6 +25,7 @@
 #include "JSObject.h"
 
 #include "ButterflyInlines.h"
+#include "CopiedBlockInlines.h"
 #include "CopiedSpaceInlines.h"
 #include "CopyVisitor.h"
 #include "CopyVisitorInlines.h"
index d875345..144b9da 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "JSSet.h"
 
+#include "CopiedBlockInlines.h"
 #include "JSCJSValueInlines.h"
 #include "JSSetIterator.h"
 #include "MapDataInlines.h"
index 068f52f..fdb3842 100644 (file)
@@ -108,7 +108,6 @@ private:
         Base::finishCreation(vm);
         m_length = length;
         setIs8Bit(m_value.impl()->is8Bit());
-        vm.m_newStringsSinceLastHashCons++;
     }
 
     void finishCreation(VM& vm, size_t length, size_t cost)
@@ -118,7 +117,6 @@ private:
         m_length = length;
         setIs8Bit(m_value.impl()->is8Bit());
         Heap::heap(this)->reportExtraMemoryAllocated(cost);
-        vm.m_newStringsSinceLastHashCons++;
     }
 
 protected:
@@ -127,7 +125,6 @@ protected:
         Base::finishCreation(vm);
         m_length = 0;
         setIs8Bit(true);
-        vm.m_newStringsSinceLastHashCons++;
     }
 
 public:
@@ -191,8 +188,6 @@ public:
     static void visitChildren(JSCell*, SlotVisitor&);
 
     enum {
-        HashConsLock = 1u << 2,
-        IsHashConsSingleton = 1u << 1,
         Is8Bit = 1u
     };
 
@@ -209,12 +204,6 @@ protected:
         else
             m_flags &= ~Is8Bit;
     }
-    bool shouldTryHashCons();
-    bool isHashConsSingleton() const { return m_flags & IsHashConsSingleton; }
-    void clearHashConsSingleton() { m_flags &= ~IsHashConsSingleton; }
-    void setHashConsSingleton() { m_flags |= IsHashConsSingleton; }
-    bool tryHashConsLock();
-    void releaseHashConsLock();
 
     mutable unsigned m_flags;
 
index 2c5c474..3a469ad 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "JSTypedArrays.h"
 
+#include "CopiedBlockInlines.h"
 #include "CopyVisitorInlines.h"
 #include "GenericTypedArrayViewInlines.h"
 #include "JSGenericTypedArrayViewInlines.h"
index 1c7fc62..b2c51dd 100644 (file)
@@ -166,7 +166,6 @@ VM::VM(VMType vmType, HeapType heapType)
 #if ENABLE(REGEXP_TRACING)
     , m_rtTraceList(new RTTraceList())
 #endif
-    , m_newStringsSinceLastHashCons(0)
 #if ENABLE(ASSEMBLER)
     , m_canUseAssembler(enableAssembler(executableAllocator))
 #endif
index 8123404..a49b94e 100644 (file)
@@ -538,13 +538,6 @@ public:
     void setInitializingObjectClass(const ClassInfo*);
 #endif
 
-    unsigned m_newStringsSinceLastHashCons;
-
-    static const unsigned s_minNumberOfNewStringsToHashCons = 100;
-
-    bool haveEnoughNewStringsToHashCons() { return m_newStringsSinceLastHashCons > s_minNumberOfNewStringsToHashCons; }
-    void resetNewStringsSinceLastHashCons() { m_newStringsSinceLastHashCons = 0; }
-
     bool currentThreadIsHoldingAPILock() const { return m_apiLock->currentThreadIsHoldingLock(); }
 
     JSLock& apiLock() { return *m_apiLock; }