Handling of opaque roots is wrong in EdenCollections
authormhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Feb 2014 03:38:19 +0000 (03:38 +0000)
committermhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Feb 2014 03:38:19 +0000 (03:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=128210

Reviewed by Oliver Hunt.

The set of opaque roots is always cleared during each collection. We should instead persist
the set of opaque roots across EdenCollections and only clear it at the beginning of FullCollections.

Also added a couple of custom objects to the jsc shell that allow us to test this.

* heap/GCThreadSharedData.cpp:
(JSC::GCThreadSharedData::reset):
(JSC::GCThreadSharedData::didStartMarking):
* heap/Heap.cpp:
(JSC::Heap::markRoots):
* heap/Heap.h:
(JSC::Heap::setShouldDoFullCollection):
* heap/SlotVisitor.cpp:
(JSC::SlotVisitor::didStartMarking):
(JSC::SlotVisitor::reset):
* heap/SlotVisitor.h:
* jsc.cpp:
(WTF::Element::Element):
(WTF::Element::root):
(WTF::Element::setRoot):
(WTF::Element::create):
(WTF::Element::createStructure):
(WTF::ElementHandleOwner::isReachableFromOpaqueRoots):
(WTF::Root::Root):
(WTF::Root::element):
(WTF::Root::setElement):
(WTF::Root::create):
(WTF::Root::createStructure):
(WTF::Root::visitChildren):
(WTF::Element::handleOwner):
(WTF::Element::finishCreation):
(GlobalObject::finishCreation):
(functionCreateRoot):
(functionCreateElement):
(functionGetElement):
(functionSetElementRoot):
(functionGCAndSweep):
(functionFullGC):
(functionEdenGC):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/GCThreadSharedData.cpp
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/SlotVisitor.cpp
Source/JavaScriptCore/heap/SlotVisitor.h
Source/JavaScriptCore/jsc.cpp

index 5d55e82..e3b3fc8 100644 (file)
@@ -1,3 +1,50 @@
+2014-02-05  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        Handling of opaque roots is wrong in EdenCollections
+        https://bugs.webkit.org/show_bug.cgi?id=128210
+
+        Reviewed by Oliver Hunt.
+
+        The set of opaque roots is always cleared during each collection. We should instead persist 
+        the set of opaque roots across EdenCollections and only clear it at the beginning of FullCollections.
+
+        Also added a couple of custom objects to the jsc shell that allow us to test this.
+
+        * heap/GCThreadSharedData.cpp:
+        (JSC::GCThreadSharedData::reset):
+        (JSC::GCThreadSharedData::didStartMarking):
+        * heap/Heap.cpp:
+        (JSC::Heap::markRoots):
+        * heap/Heap.h:
+        (JSC::Heap::setShouldDoFullCollection):
+        * heap/SlotVisitor.cpp:
+        (JSC::SlotVisitor::didStartMarking):
+        (JSC::SlotVisitor::reset):
+        * heap/SlotVisitor.h:
+        * jsc.cpp:
+        (WTF::Element::Element):
+        (WTF::Element::root):
+        (WTF::Element::setRoot):
+        (WTF::Element::create):
+        (WTF::Element::createStructure):
+        (WTF::ElementHandleOwner::isReachableFromOpaqueRoots):
+        (WTF::Root::Root):
+        (WTF::Root::element):
+        (WTF::Root::setElement):
+        (WTF::Root::create):
+        (WTF::Root::createStructure):
+        (WTF::Root::visitChildren):
+        (WTF::Element::handleOwner):
+        (WTF::Element::finishCreation):
+        (GlobalObject::finishCreation):
+        (functionCreateRoot):
+        (functionCreateElement):
+        (functionGetElement):
+        (functionSetElementRoot):
+        (functionGCAndSweep):
+        (functionFullGC):
+        (functionEdenGC):
+
 2014-02-05  Anders Carlsson  <andersca@apple.com>
 
         Remove unused functions.
index 09143a1..6f778ab 100644 (file)
@@ -118,16 +118,11 @@ GCThreadSharedData::~GCThreadSharedData()
     }
 #endif
 }
-    
+
 void GCThreadSharedData::reset()
 {
     ASSERT(m_sharedMarkStack.isEmpty());
     
-#if ENABLE(PARALLEL_GC)
-    m_opaqueRoots.clear();
-#else
-    ASSERT(m_opaqueRoots.isEmpty());
-#endif
     m_weakReferenceHarvesters.removeAll();
 
     if (m_shouldHashCons) {
@@ -158,6 +153,13 @@ void GCThreadSharedData::endCurrentPhase()
 
 void GCThreadSharedData::didStartMarking()
 {
+    if (m_vm->heap.operationInProgress() == FullCollection) {
+#if ENABLE(PARALLEL_GC)
+        m_opaqueRoots.clear();
+#else
+        ASSERT(m_opaqueRoots.isEmpty());
+#endif
+}
     std::lock_guard<std::mutex> lock(m_markingMutex);
     m_parallelMarkersShouldExit = false;
     startNextPhase(Mark);
index 11ecdeb..e44cd12 100644 (file)
@@ -490,7 +490,7 @@ void Heap::markRoots()
 
     m_sharedData.didStartMarking();
     SlotVisitor& visitor = m_slotVisitor;
-    visitor.setup();
+    visitor.didStartMarking();
     HeapRootVisitor heapRootVisitor(visitor);
 
 #if ENABLE(GGC)
index 6ae74cf..365a41f 100644 (file)
@@ -148,7 +148,8 @@ namespace JSC {
         JS_EXPORT_PRIVATE void collectAllGarbage();
         bool shouldCollect();
         void gcTimerDidFire() { m_shouldDoFullCollection = true; }
-        void collect();
+        void setShouldDoFullCollection(bool shouldDoFullCollection) { m_shouldDoFullCollection = shouldDoFullCollection; }
+        JS_EXPORT_PRIVATE void collect();
         bool collectIfNecessaryOrDefer(); // Returns true if it did collect.
 
         void reportExtraMemoryCost(size_t cost);
index 4fd0da7..b912925 100644 (file)
@@ -36,8 +36,16 @@ SlotVisitor::~SlotVisitor()
     clearMarkStack();
 }
 
-void SlotVisitor::setup()
+void SlotVisitor::didStartMarking()
 {
+    if (heap()->operationInProgress() == FullCollection) {
+#if ENABLE(PARALLEL_GC)
+        ASSERT(m_opaqueRoots.isEmpty()); // Should have merged by now.
+#else
+        m_opaqueRoots.clear();
+#endif
+    }
+
     m_shared.m_shouldHashCons = m_shared.m_vm->haveEnoughNewStringsToHashCons();
     m_shouldHashCons = m_shared.m_shouldHashCons;
 #if ENABLE(PARALLEL_GC)
@@ -52,11 +60,6 @@ void SlotVisitor::reset()
     m_bytesCopied = 0;
     m_visitCount = 0;
     ASSERT(m_stack.isEmpty());
-#if ENABLE(PARALLEL_GC)
-    ASSERT(m_opaqueRoots.isEmpty()); // Should have merged by now.
-#else
-    m_opaqueRoots.clear();
-#endif
     if (m_shouldHashCons) {
         m_uniqueStrings.clear();
         m_shouldHashCons = false;
index 363134f..bad21a7 100644 (file)
@@ -78,7 +78,7 @@ public:
     GCThreadSharedData& sharedData() const { return m_shared; }
     bool isEmpty() { return m_stack.isEmpty(); }
 
-    void setup();
+    void didStartMarking();
     void reset();
     void clearMarkStack();
 
index f9d1542..3ccfe4d 100644 (file)
 using namespace JSC;
 using namespace WTF;
 
+namespace {
+
+class Element;
+class ElementHandleOwner;
+class Root;
+
+class Element : public JSNonFinalObject {
+public:
+    Element(VM& vm, Structure* structure, Root* root)
+        : Base(vm, structure)
+        , m_root(root)
+    {
+    }
+
+    typedef JSNonFinalObject Base;
+    static const bool needsDestruction = false;
+
+    Root* root() const { return m_root; }
+    void setRoot(Root* root) { m_root = root; }
+
+    static Element* create(VM& vm, JSGlobalObject* globalObject, Root* root)
+    {
+        Structure* structure = createStructure(vm, globalObject, jsNull());
+        Element* element = new (NotNull, allocateCell<Element>(vm.heap, sizeof(Element))) Element(vm, structure, root);
+        element->finishCreation(vm);
+        return element;
+    }
+
+    void finishCreation(VM&);
+
+    static ElementHandleOwner* handleOwner();
+
+    static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
+    {
+        return Structure::create(vm, globalObject, prototype, TypeInfo(ObjectType, StructureFlags), info());
+    }
+
+    DECLARE_INFO;
+
+private:
+    Root* m_root;
+};
+
+class ElementHandleOwner : public WeakHandleOwner {
+public:
+    virtual bool isReachableFromOpaqueRoots(Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor)
+    {
+        Element* element = jsCast<Element*>(handle.slot()->asCell());
+        return visitor.containsOpaqueRoot(element->root());
+    }
+};
+
+class Root : public JSDestructibleObject {
+public:
+    Root(VM& vm, Structure* structure)
+        : Base(vm, structure)
+    {
+    }
+
+    Element* element()
+    {
+        return m_element.get();
+    }
+
+    void setElement(Element* element)
+    {
+        Weak<Element> newElement(element, Element::handleOwner());
+        m_element.swap(newElement);
+    }
+
+    static Root* create(VM& vm, JSGlobalObject* globalObject)
+    {
+        Structure* structure = createStructure(vm, globalObject, jsNull());
+        Root* root = new (NotNull, allocateCell<Root>(vm.heap, sizeof(Root))) Root(vm, structure);
+        root->finishCreation(vm);
+        return root;
+    }
+
+    typedef JSDestructibleObject Base;
+
+    DECLARE_INFO;
+    static const bool needsDestruction = true;
+
+    static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
+    {
+        return Structure::create(vm, globalObject, prototype, TypeInfo(ObjectType, StructureFlags), info());
+    }
+
+    static void visitChildren(JSCell* thisObject, SlotVisitor& visitor)
+    {
+        Base::visitChildren(thisObject, visitor);
+        visitor.addOpaqueRoot(thisObject);
+    }
+
+private:
+    Weak<Element> m_element;
+};
+
+const ClassInfo Element::s_info = { "Element", &Base::s_info, 0, 0, CREATE_METHOD_TABLE(Element) };
+const ClassInfo Root::s_info = { "Root", &Base::s_info, 0, 0, CREATE_METHOD_TABLE(Root) };
+
+ElementHandleOwner* Element::handleOwner()
+{
+    static ElementHandleOwner* owner = 0;
+    if (!owner)
+        owner = new ElementHandleOwner();
+    return owner;
+}
+
+void Element::finishCreation(VM& vm)
+{
+    Base::finishCreation(vm);
+    m_root->setElement(this);
+}
+
+}
+
 static bool fillBufferWithContentsOfFile(const String& fileName, Vector<char>& buffer);
 
+static EncodedJSValue JSC_HOST_CALL functionSetElementRoot(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionCreateRoot(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionCreateElement(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionGetElement(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionPrint(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionDebug(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionDescribe(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionJSCStack(ExecState*);
-static EncodedJSValue JSC_HOST_CALL functionGC(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionGCAndSweep(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionFullGC(ExecState*);
+static EncodedJSValue JSC_HOST_CALL functionEdenGC(ExecState*);
 #ifndef NDEBUG
 static EncodedJSValue JSC_HOST_CALL functionReleaseExecutableMemory(ExecState*);
 static EncodedJSValue JSC_HOST_CALL functionDumpCallFrame(ExecState*);
@@ -219,7 +342,9 @@ protected:
         addFunction(vm, "describe", functionDescribe, 1);
         addFunction(vm, "print", functionPrint, 1);
         addFunction(vm, "quit", functionQuit, 0);
-        addFunction(vm, "gc", functionGC, 0);
+        addFunction(vm, "gc", functionGCAndSweep, 0);
+        addFunction(vm, "fullGC", functionFullGC, 0);
+        addFunction(vm, "edenGC", functionEdenGC, 0);
 #ifndef NDEBUG
         addFunction(vm, "dumpCallFrame", functionDumpCallFrame, 0);
         addFunction(vm, "releaseExecutableMemory", functionReleaseExecutableMemory, 0);
@@ -241,6 +366,10 @@ protected:
         addFunction(vm, "setSamplingFlags", functionSetSamplingFlags, 1);
         addFunction(vm, "clearSamplingFlags", functionClearSamplingFlags, 1);
 #endif
+        addConstructableFunction(vm, "Root", functionCreateRoot, 0);
+        addConstructableFunction(vm, "Element", functionCreateElement, 1);
+        addFunction(vm, "getElement", functionGetElement, 1);
+        addFunction(vm, "setElementRoot", functionSetElementRoot, 2);
         
         JSArray* array = constructEmptyArray(globalExec(), 0);
         for (size_t i = 0; i < arguments.size(); ++i)
@@ -357,13 +486,60 @@ EncodedJSValue JSC_HOST_CALL functionJSCStack(ExecState* exec)
     return JSValue::encode(jsUndefined());
 }
 
-EncodedJSValue JSC_HOST_CALL functionGC(ExecState* exec)
+EncodedJSValue JSC_HOST_CALL functionCreateRoot(ExecState* exec)
+{
+    JSLockHolder lock(exec);
+    return JSValue::encode(Root::create(exec->vm(), exec->lexicalGlobalObject()));
+}
+
+EncodedJSValue JSC_HOST_CALL functionCreateElement(ExecState* exec)
+{
+    JSLockHolder lock(exec);
+    JSValue arg = exec->argument(0);
+    return JSValue::encode(Element::create(exec->vm(), exec->lexicalGlobalObject(), arg.isNull() ? nullptr : jsCast<Root*>(exec->argument(0))));
+}
+
+EncodedJSValue JSC_HOST_CALL functionGetElement(ExecState* exec)
+{
+    JSLockHolder lock(exec);
+    Element* result = jsCast<Root*>(exec->argument(0).asCell())->element();
+    return JSValue::encode(result ? result : jsUndefined());
+}
+
+EncodedJSValue JSC_HOST_CALL functionSetElementRoot(ExecState* exec)
+{
+    JSLockHolder lock(exec);
+    Element* element = jsCast<Element*>(exec->argument(0));
+    Root* root = jsCast<Root*>(exec->argument(1));
+    element->setRoot(root);
+    return JSValue::encode(jsUndefined());
+}
+
+EncodedJSValue JSC_HOST_CALL functionGCAndSweep(ExecState* exec)
 {
     JSLockHolder lock(exec);
     exec->heap()->collectAllGarbage();
     return JSValue::encode(jsUndefined());
 }
 
+EncodedJSValue JSC_HOST_CALL functionFullGC(ExecState* exec)
+{
+    JSLockHolder lock(exec);
+    Heap* heap = exec->heap();
+    heap->setShouldDoFullCollection(true);
+    exec->heap()->collect();
+    return JSValue::encode(jsUndefined());
+}
+
+EncodedJSValue JSC_HOST_CALL functionEdenGC(ExecState* exec)
+{
+    JSLockHolder lock(exec);
+    Heap* heap = exec->heap();
+    heap->setShouldDoFullCollection(false);
+    heap->collect();
+    return JSValue::encode(jsUndefined());
+}
+
 #ifndef NDEBUG
 EncodedJSValue JSC_HOST_CALL functionReleaseExecutableMemory(ExecState* exec)
 {