Merge r181297 - Stale entries in WeakGCMaps are keeping tons of WeakB…

…locks alive unnecessarily.

<https://webkit.org/b/142115>
<rdar://problem/19992268>

Reviewed by Geoffrey Garen.

Prune stale entries from WeakGCMaps as part of every full garbage collection.
This frees up tons of previously-stuck WeakBlocks that were only sitting around
with finalized handles waiting to die.

Note that WeakGCMaps register/unregister themselves with the GC heap in their
ctor/dtor, so creating one now requires passing the VM.

Average time spent in the PruningStaleEntriesFromWeakGCMaps GC phase appears
to be between 0.01ms and 0.3ms, though I've seen a few longer ones at ~1.2ms.
It seems somewhat excessive to do this on every Eden collection, so it's only
doing work in full collections for now.

Because the GC may now mutate WeakGCMap below object allocation, I've made it
so that the classic HashMap::add() optimization can't be used with WeakGCMap.
This caused intermittent test failures when originally landed due to having
an invalid iterator on the stack after add() inserted a new entry and we
proceeded to allocate the new object, triggering GC.

* API/JSWeakObjectMapRefInternal.h:
(OpaqueJSWeakObjectMap::create):
(OpaqueJSWeakObjectMap::OpaqueJSWeakObjectMap):
* API/JSWeakObjectMapRefPrivate.cpp:
* API/JSWrapperMap.mm:
(-[JSWrapperMap initWithContext:]):
(-[JSWrapperMap jsWrapperForObject:]): Pass VM to WeakGCMap constructor.

* JavaScriptCore.xcodeproj/project.pbxproj: Add WeakGCMapInlines.h and make
it project-private so WebCore clients can access it.

* heap/Heap.cpp:
(JSC::Heap::collect):
(JSC::Heap::pruneStaleEntriesFromWeakGCMaps): Added a new GC phase for pruning
stale entries from WeakGCMaps. This is only executed during full collections.

* heap/Heap.h:
* heap/HeapInlines.h:
(JSC::Heap::registerWeakGCMap):
(JSC::Heap::unregisterWeakGCMap): Added a mechanism for WeakGCMaps to register
themselves with the Heap and provide a pruning callback.

* runtime/PrototypeMap.h:
(JSC::PrototypeMap::PrototypeMap):
* runtime/Structure.cpp:
(JSC::StructureTransitionTable::add): Pass VM to WeakGCMap constructor.

* runtime/JSCInlines.h: Add "WeakGCMapInlines.h"

* runtime/JSGlobalObject.cpp: Include "WeakGCMapInlines.h" so this builds.

* runtime/JSString.cpp:
(JSC::jsStringWithCacheSlowCase):
* runtime/PrototypeMap.cpp:
(JSC::PrototypeMap::addPrototype):
(JSC::PrototypeMap::emptyObjectStructureForPrototype): Remove HashMap add()
optimization since it's not safe in the GC-managed WeakGCMap world.

* runtime/VM.cpp:
(JSC::VM::VM): Pass VM to WeakGCMap constructor.

* runtime/WeakGCMap.h:
(JSC::WeakGCMap::set):
(JSC::WeakGCMap::add):
(JSC::WeakGCMap::WeakGCMap): Deleted.
(JSC::WeakGCMap::gcMap): Deleted.
(JSC::WeakGCMap::gcMapIfNeeded): Deleted.
* runtime/WeakGCMapInlines.h: Added.
(JSC::WeakGCMap::WeakGCMap):
(JSC::WeakGCMap::~WeakGCMap):
(JSC::WeakGCMap::pruneStaleEntries): Moved ctor, dtor and pruning callback
to WeakGCMapInlines.h to fix interdependent header issues. Removed code that
prunes WeakGCMap at certain growth milestones and instead rely on the GC
callback for housekeeping.

Source/JavaScriptCore/API/JSWeakObjectMapRefInternal.h

@@ -41,9 +41,9 @@ typedef JSC::WeakGCMap<void*, JSC::JSObject> WeakMapType;
 
 struct OpaqueJSWeakObjectMap : public RefCounted<OpaqueJSWeakObjectMap> {
 public:
-    static PassRefPtr<OpaqueJSWeakObjectMap> create(void* data, JSWeakMapDestroyedCallback callback)
+    static Ref<OpaqueJSWeakObjectMap> create(JSC::VM& vm, void* data, JSWeakMapDestroyedCallback callback)
     {
-        return adoptRef(new OpaqueJSWeakObjectMap(data, callback));
+        return adoptRef(*new OpaqueJSWeakObjectMap(vm, data, callback));
     }
 
     WeakMapType& map() { return m_map; }
@@ -54,8 +54,9 @@ struct OpaqueJSWeakObjectMap : public RefCounted<OpaqueJSWeakObjectMap> {
     }
 
 private:
-    OpaqueJSWeakObjectMap(void* data, JSWeakMapDestroyedCallback callback)
-        : m_data(data)
+    OpaqueJSWeakObjectMap(JSC::VM& vm, void* data, JSWeakMapDestroyedCallback callback)
+        : m_map(vm)
+        , m_data(data)
         , m_callback(callback)
     {
     }

Source/JavaScriptCore/API/JSWeakObjectMapRefPrivate.cpp

@@ -32,6 +32,7 @@
 #include "JSWeakObjectMapRefInternal.h"
 #include "JSCInlines.h"
 #include "Weak.h"
+#include "WeakGCMapInlines.h"
 #include <wtf/HashMap.h>
 #include <wtf/text/StringHash.h>
 
@@ -46,7 +47,7 @@ JSWeakObjectMapRef JSWeakObjectMapCreate(JSContextRef context, void* privateData
 {
     ExecState* exec = toJS(context);
     JSLockHolder locker(exec);
-    RefPtr<OpaqueJSWeakObjectMap> map = OpaqueJSWeakObjectMap::create(privateData, callback);
+    RefPtr<OpaqueJSWeakObjectMap> map = OpaqueJSWeakObjectMap::create(exec->vm(), privateData, callback);
     exec->lexicalGlobalObject()->registerWeakMap(map.get());
     return map.get();
 }

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,84 @@
+2015-03-09  Andreas Kling  <akling@apple.com>
+
+        Stale entries in WeakGCMaps are keeping tons of WeakBlocks alive unnecessarily.
+        <https://webkit.org/b/142115>
+        <rdar://problem/19992268>
+
+        Reviewed by Geoffrey Garen.
+
+        Prune stale entries from WeakGCMaps as part of every full garbage collection.
+        This frees up tons of previously-stuck WeakBlocks that were only sitting around
+        with finalized handles waiting to die.
+
+        Note that WeakGCMaps register/unregister themselves with the GC heap in their
+        ctor/dtor, so creating one now requires passing the VM.
+
+        Average time spent in the PruningStaleEntriesFromWeakGCMaps GC phase appears
+        to be between 0.01ms and 0.3ms, though I've seen a few longer ones at ~1.2ms.
+        It seems somewhat excessive to do this on every Eden collection, so it's only
+        doing work in full collections for now.
+
+        Because the GC may now mutate WeakGCMap below object allocation, I've made it
+        so that the classic HashMap::add() optimization can't be used with WeakGCMap.
+        This caused intermittent test failures when originally landed due to having
+        an invalid iterator on the stack after add() inserted a new entry and we
+        proceeded to allocate the new object, triggering GC.
+
+        * API/JSWeakObjectMapRefInternal.h:
+        (OpaqueJSWeakObjectMap::create):
+        (OpaqueJSWeakObjectMap::OpaqueJSWeakObjectMap):
+        * API/JSWeakObjectMapRefPrivate.cpp:
+        * API/JSWrapperMap.mm:
+        (-[JSWrapperMap initWithContext:]):
+        (-[JSWrapperMap jsWrapperForObject:]): Pass VM to WeakGCMap constructor.
+
+        * JavaScriptCore.xcodeproj/project.pbxproj: Add WeakGCMapInlines.h and make
+        it project-private so WebCore clients can access it.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::collect):
+        (JSC::Heap::pruneStaleEntriesFromWeakGCMaps): Added a new GC phase for pruning
+        stale entries from WeakGCMaps. This is only executed during full collections.
+
+        * heap/Heap.h:
+        * heap/HeapInlines.h:
+        (JSC::Heap::registerWeakGCMap):
+        (JSC::Heap::unregisterWeakGCMap): Added a mechanism for WeakGCMaps to register
+        themselves with the Heap and provide a pruning callback.
+
+        * runtime/PrototypeMap.h:
+        (JSC::PrototypeMap::PrototypeMap):
+        * runtime/Structure.cpp:
+        (JSC::StructureTransitionTable::add): Pass VM to WeakGCMap constructor.
+
+        * runtime/JSCInlines.h: Add "WeakGCMapInlines.h"
+
+        * runtime/JSGlobalObject.cpp: Include "WeakGCMapInlines.h" so this builds.
+
+        * runtime/JSString.cpp:
+        (JSC::jsStringWithCacheSlowCase):
+        * runtime/PrototypeMap.cpp:
+        (JSC::PrototypeMap::addPrototype):
+        (JSC::PrototypeMap::emptyObjectStructureForPrototype): Remove HashMap add()
+        optimization since it's not safe in the GC-managed WeakGCMap world.
+
+        * runtime/VM.cpp:
+        (JSC::VM::VM): Pass VM to WeakGCMap constructor.
+
+        * runtime/WeakGCMap.h:
+        (JSC::WeakGCMap::set):
+        (JSC::WeakGCMap::add):
+        (JSC::WeakGCMap::WeakGCMap): Deleted.
+        (JSC::WeakGCMap::gcMap): Deleted.
+        (JSC::WeakGCMap::gcMapIfNeeded): Deleted.
+        * runtime/WeakGCMapInlines.h: Added.
+        (JSC::WeakGCMap::WeakGCMap):
+        (JSC::WeakGCMap::~WeakGCMap):
+        (JSC::WeakGCMap::pruneStaleEntries): Moved ctor, dtor and pruning callback
+        to WeakGCMapInlines.h to fix interdependent header issues. Removed code that
+        prunes WeakGCMap at certain growth milestones and instead rely on the GC
+        callback for housekeeping.
+
 2015-03-05  Chris Dumez  <cdumez@apple.com>
 
         Regression(r173761): ASSERTION FAILED: !is8Bit() in StringImpl::characters16()

Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj

@@ -1418,6 +1418,7 @@
 		A7FB61001040C38B0017A286 /* PropertyDescriptor.h in Headers */ = {isa = PBXBuildFile; fileRef = A7FB604B103F5EAB0017A286 /* PropertyDescriptor.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		A7FCC26D17A0B6AA00786D1A /* FTLSwitchCase.h in Headers */ = {isa = PBXBuildFile; fileRef = A7FCC26C17A0B6AA00786D1A /* FTLSwitchCase.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		A8A4748E151A8306004123FF /* libWTF.a in Frameworks */ = {isa = PBXBuildFile; fileRef = A8A4748D151A8306004123FF /* libWTF.a */; };
+		AD86A93E1AA4D88D002FE77F /* WeakGCMapInlines.h in Headers */ = {isa = PBXBuildFile; fileRef = AD86A93D1AA4D87C002FE77F /* WeakGCMapInlines.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		ADDB1F6318D77DBE009B58A8 /* OpaqueRootSet.h in Headers */ = {isa = PBXBuildFile; fileRef = ADDB1F6218D77DB7009B58A8 /* OpaqueRootSet.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		ADE39FFF16DD144B0003CD4A /* PropertyTable.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AD1CF06816DCAB2D00B97123 /* PropertyTable.cpp */; };
 		B59F89391891F29F00D5CCDC /* UnlinkedInstructionStream.cpp in Sources */ = {isa = PBXBuildFile; fileRef = B59F89381891ADB500D5CCDC /* UnlinkedInstructionStream.cpp */; };
@@ -3126,6 +3127,7 @@
 		A8E894310CD0602400367179 /* JSCallbackObjectFunctions.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSCallbackObjectFunctions.h; sourceTree = "<group>"; };
 		A8E894330CD0603F00367179 /* JSGlobalObject.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSGlobalObject.h; sourceTree = "<group>"; };
 		AD1CF06816DCAB2D00B97123 /* PropertyTable.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = PropertyTable.cpp; sourceTree = "<group>"; };
+		AD86A93D1AA4D87C002FE77F /* WeakGCMapInlines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WeakGCMapInlines.h; sourceTree = "<group>"; };
 		ADDB1F6218D77DB7009B58A8 /* OpaqueRootSet.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OpaqueRootSet.h; sourceTree = "<group>"; };
 		B59F89371891AD3300D5CCDC /* UnlinkedInstructionStream.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = UnlinkedInstructionStream.h; sourceTree = "<group>"; };
 		B59F89381891ADB500D5CCDC /* UnlinkedInstructionStream.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = UnlinkedInstructionStream.cpp; sourceTree = "<group>"; };
@@ -4598,6 +4600,7 @@
 				FED94F2C171E3E2300BE77A4 /* Watchdog.h */,
 				FED94F2D171E3E2300BE77A4 /* WatchdogMac.cpp */,
 				14BFCE6810CDB1FC00364CCE /* WeakGCMap.h */,
+				AD86A93D1AA4D87C002FE77F /* WeakGCMapInlines.h */,
 				A7CA3ADD17DA41AE006538AF /* WeakMapConstructor.cpp */,
 				A7CA3ADE17DA41AE006538AF /* WeakMapConstructor.h */,
 				A7CA3AE917DA5168006538AF /* WeakMapData.cpp */,
@@ -5492,6 +5495,7 @@
 				0F885E111849A3BE00F1E3FA /* BytecodeUseDef.h in Headers */,
 				0F8023EA1613832B00A0BA45 /* ByValInfo.h in Headers */,
 				BC18C3ED0E16F5CD00B34460 /* CallData.h in Headers */,
+				AD86A93E1AA4D88D002FE77F /* WeakGCMapInlines.h in Headers */,
 				1429D8DE0ED2205B00B89619 /* CallFrame.h in Headers */,
 				A7C1EAEF17987AB600299DB2 /* CallFrameInlines.h in Headers */,
 				95E3BC050E1AE68200B2D1C1 /* CallIdentifier.h in Headers */,

Source/JavaScriptCore/heap/Heap.cpp

@@ -1051,6 +1051,7 @@ NEVER_INLINE void Heap::collectImpl(HeapOperation collectionType, void* stackOri
         vm()->typeProfiler()->invalidateTypeSetCache();
 
     reapWeakHandles();
+    pruneStaleEntriesFromWeakGCMaps();
     sweepArrayBuffers();
     snapshotMarkedSpace();
 
@@ -1164,6 +1165,15 @@ void Heap::reapWeakHandles()
     m_objectSpace.reapWeakSets();
 }
 
+void Heap::pruneStaleEntriesFromWeakGCMaps()
+{
+    GCPHASE(PruningStaleEntriesFromWeakGCMaps);
+    if (m_operationInProgress != FullCollection)
+        return;
+    for (auto& pruneCallback : m_weakGCMaps.values())
+        pruneCallback();
+}
+
 void Heap::sweepArrayBuffers()
 {
     GCPHASE(SweepingArrayBuffers);

Source/JavaScriptCore/heap/Heap.h

@@ -227,6 +227,9 @@ class Heap {
 
     static bool isZombified(JSCell* cell) { return *(void**)cell == zombifiedBits; }
 
+    void registerWeakGCMap(void* weakGCMap, std::function<void()> pruningCallback);
+    void unregisterWeakGCMap(void* weakGCMap);
+
 private:
     friend class CodeBlock;
     friend class CopiedBlock;
@@ -300,6 +303,7 @@ class Heap {
     void resetVisitors();
 
     void reapWeakHandles();
+    void pruneStaleEntriesFromWeakGCMaps();
     void sweepArrayBuffers();
     void snapshotMarkedSpace();
     void deleteSourceProviderCaches();
@@ -393,6 +397,8 @@ class Heap {
     Vector<RetainPtr<CFTypeRef>> m_delayedReleaseObjects;
     unsigned m_delayedReleaseRecursionCount;
 #endif
+
+    HashMap<void*, std::function<void()>> m_weakGCMaps;
 };
 
 } // namespace JSC

Source/JavaScriptCore/heap/HeapInlines.h

@@ -289,6 +289,16 @@ inline HashSet<MarkedArgumentBuffer*>& Heap::markListSet()
         m_markListSet = std::make_unique<HashSet<MarkedArgumentBuffer*>>();
     return *m_markListSet;
 }
+
+inline void Heap::registerWeakGCMap(void* weakGCMap, std::function<void()> pruningCallback)
+{
+    m_weakGCMaps.add(weakGCMap, WTF::move(pruningCallback));
+}
+
+inline void Heap::unregisterWeakGCMap(void* weakGCMap)
+{
+    m_weakGCMaps.remove(weakGCMap);
+}
 
 } // namespace JSC
 

Source/JavaScriptCore/runtime/JSCInlines.h

@@ -51,5 +51,6 @@
 #include "Operations.h"
 #include "SlotVisitorInlines.h"
 #include "StructureInlines.h"
+#include "WeakGCMapInlines.h"
 
 #endif // JSCInlines_h

Source/JavaScriptCore/runtime/JSGlobalObject.cpp

@@ -119,6 +119,7 @@
 #include "SymbolConstructor.h"
 #include "SymbolPrototype.h"
 #include "VariableWatchpointSetInlines.h"
+#include "WeakGCMapInlines.h"
 #include "WeakMapConstructor.h"
 #include "WeakMapPrototype.h"
 

Source/JavaScriptCore/runtime/JSString.cpp

@@ -437,11 +437,12 @@ bool JSString::getStringPropertyDescriptor(ExecState* exec, PropertyName propert
 
 JSString* jsStringWithCacheSlowCase(VM& vm, StringImpl& stringImpl)
 {
-    auto addResult = vm.stringCache.add(&stringImpl, nullptr);
-    if (addResult.isNewEntry)
-        addResult.iterator->value = jsString(&vm, String(stringImpl));
-    vm.lastCachedString.set(vm, addResult.iterator->value.get());
-    return addResult.iterator->value.get();
+    if (JSString* string = vm.stringCache.get(&stringImpl))
+        return string;
+
+    JSString* string = jsString(&vm, String(stringImpl));
+    vm.lastCachedString.set(vm, string);
+    return string;
 }
 
 } // namespace JSC

Source/JavaScriptCore/runtime/PrototypeMap.cpp

@@ -33,7 +33,7 @@ namespace JSC {
 
 void PrototypeMap::addPrototype(JSObject* object)
 {
-    m_prototypes.add(object, object);
+    m_prototypes.set(object, object);
 
     // Note that this method makes the somewhat odd decision to not check if this
     // object currently has indexed accessors. We could do that check here, and if
@@ -54,16 +54,16 @@ void PrototypeMap::addPrototype(JSObject* object)
 
 Structure* PrototypeMap::emptyObjectStructureForPrototype(JSObject* prototype, unsigned inlineCapacity)
 {
-    StructureMap::AddResult addResult = m_structures.add(std::make_pair(prototype, inlineCapacity), nullptr);
-    if (!addResult.isNewEntry) {
+    auto key = std::make_pair(prototype, inlineCapacity);
+    if (Structure* structure = m_structures.get(key)) {
         ASSERT(isPrototype(prototype));
-        return addResult.iterator->value.get();
+        return structure;
     }
 
     addPrototype(prototype);
     Structure* structure = JSFinalObject::createStructure(
         prototype->globalObject()->vm(), prototype->globalObject(), prototype, inlineCapacity);
-    addResult.iterator->value = Weak<Structure>(structure);
+    m_structures.set(key, Weak<Structure>(structure));
     return structure;
 }
 

Source/JavaScriptCore/runtime/PrototypeMap.h

@@ -33,10 +33,17 @@ namespace JSC {
 
 class JSObject;
 class Structure;
+class VM;
 
 // Tracks the canonical structure an object should be allocated with when inheriting from a given prototype.
 class PrototypeMap {
 public:
+    explicit PrototypeMap(VM& vm)
+        : m_prototypes(vm)
+        , m_structures(vm)
+    {
+    }
+
     JS_EXPORT_PRIVATE Structure* emptyObjectStructureForPrototype(JSObject*, unsigned inlineCapacity);
     void clearEmptyObjectStructureForPrototype(JSObject*, unsigned inlineCapacity);
     void addPrototype(JSObject*);

Source/JavaScriptCore/runtime/Structure.cpp

@@ -36,6 +36,7 @@
 #include "PropertyNameArray.h"
 #include "StructureChain.h"
 #include "StructureRareDataInlines.h"
+#include "WeakGCMapInlines.h"
 #include <wtf/CommaPrinter.h>
 #include <wtf/ProcessID.h>
 #include <wtf/RefCountedLeakCounter.h>
@@ -90,7 +91,7 @@ inline void StructureTransitionTable::add(VM& vm, Structure* structure)
 
         // This handles the second transition being added
         // (or the first transition being despecified!)
-        setMap(new TransitionMap());
+        setMap(new TransitionMap(vm));
         add(vm, existingTransition);
     }
 

Source/JavaScriptCore/runtime/VM.cpp

@@ -83,6 +83,7 @@
 #include "TypeProfiler.h"
 #include "TypeProfilerLog.h"
 #include "UnlinkedCodeBlock.h"
+#include "WeakGCMapInlines.h"
 #include "WeakMapData.h"
 #include <wtf/ProcessID.h>
 #include <wtf/RetainPtr.h>
@@ -153,6 +154,8 @@ VM::VM(VMType vmType, HeapType heapType)
     , m_atomicStringTable(vmType == Default ? wtfThreadData().atomicStringTable() : new AtomicStringTable)
     , propertyNames(nullptr)
     , emptyList(new MarkedArgumentBuffer)
+    , stringCache(*this)
+    , prototypeMap(*this)
     , keywords(std::make_unique<Keywords>(*this))
     , interpreter(0)
     , jsArrayClassInfo(JSArray::info())