Bug 982561 - Convert JSCompartment's list of marked weakmaps into list of all weakmaps r=terrence

js/src/jsgc.cpp

@@ -2972,8 +2972,8 @@ GCRuntime::beginMarkPhase()
     }
 
     for (GCCompartmentsIter c(rt); !c.done(); c.next()) {
-        /* Reset weak map list for the compartments being collected. */
+        /* Unmark all weak maps in the compartments being collected. */
-        WeakMapBase::resetCompartmentWeakMapList(c);
+        WeakMapBase::unmarkCompartment(c);
     }
 
     if (isFull)
@@ -3173,14 +3173,17 @@ js::gc::MarkingValidator::nonIncrementalMark()
     }
 
     /*
-     * Temporarily clear the lists of live weakmaps and array buffers for the
+     * Temporarily clear the weakmaps' mark flags and the lists of live array
-     * compartments we are collecting.
+     * buffers for the compartments we are collecting.
      */
 
-    WeakMapVector weakmaps;
+    WeakMapSet markedWeakMaps;
+    if (!markedWeakMaps.init())
+        return;
+
     ArrayBufferVector arrayBuffers;
     for (GCCompartmentsIter c(runtime); !c.done(); c.next()) {
-        if (!WeakMapBase::saveCompartmentWeakMapList(c, weakmaps) ||
+        if (!WeakMapBase::saveCompartmentMarkedWeakMaps(c, markedWeakMaps) ||
             !ArrayBufferObject::saveArrayBufferList(c, arrayBuffers))
         {
             return;
@@ -3194,7 +3197,7 @@ js::gc::MarkingValidator::nonIncrementalMark()
     initialized = true;
 
     for (GCCompartmentsIter c(runtime); !c.done(); c.next()) {
-        WeakMapBase::resetCompartmentWeakMapList(c);
+        WeakMapBase::unmarkCompartment(c);
         ArrayBufferObject::resetArrayBufferList(c);
     }
 
@@ -3250,10 +3253,10 @@ js::gc::MarkingValidator::nonIncrementalMark()
     }
 
     for (GCCompartmentsIter c(runtime); !c.done(); c.next()) {
-        WeakMapBase::resetCompartmentWeakMapList(c);
+        WeakMapBase::unmarkCompartment(c);
         ArrayBufferObject::resetArrayBufferList(c);
     }
-    WeakMapBase::restoreCompartmentWeakMapLists(weakmaps);
+    WeakMapBase::restoreCompartmentMarkedWeakMaps(markedWeakMaps);
     ArrayBufferObject::restoreArrayBufferLists(arrayBuffers);
 
     gc->incrementalState = state;

js/src/jsweakmap.cpp

@@ -24,14 +24,47 @@ using namespace js;
 WeakMapBase::WeakMapBase(JSObject *memOf, JSCompartment *c)
   : memberOf(memOf),
     compartment(c),
-    next(WeakMapNotInList)
+    next(WeakMapNotInList),
+    marked(false)
 {
     JS_ASSERT_IF(memberOf, memberOf->compartment() == c);
 }
 
 WeakMapBase::~WeakMapBase()
 {
-    JS_ASSERT(next == WeakMapNotInList);
+    JS_ASSERT(!isInList());
+}
+
+void
+WeakMapBase::trace(JSTracer *tracer)
+{
+    JS_ASSERT(isInList());
+    if (IS_GC_MARKING_TRACER(tracer)) {
+        // We don't trace any of the WeakMap entries at this time, just record
+        // record the fact that the WeakMap has been marked. Enties are marked
+        // in the iterative marking phase by markAllIteratively(), which happens
+        // when many keys as possible have been marked already.
+        JS_ASSERT(tracer->eagerlyTraceWeakMaps() == DoNotTraceWeakMaps);
+        marked = true;
+    } else {
+        // If we're not actually doing garbage collection, the keys won't be marked
+        // nicely as needed by the true ephemeral marking algorithm --- custom tracers
+        // such as the cycle collector must use their own means for cycle detection.
+        // So here we do a conservative approximation: pretend all keys are live.
+        if (tracer->eagerlyTraceWeakMaps() == DoNotTraceWeakMaps)
+            return;
+
+        nonMarkingTraceValues(tracer);
+        if (tracer->eagerlyTraceWeakMaps() == TraceWeakMapKeysValues)
+            nonMarkingTraceKeys(tracer);
+    }
+}
+
+void
+WeakMapBase::unmarkCompartment(JSCompartment *c)
+{
+    for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next)
+        m->marked = false;
 }
 
 bool
@@ -39,7 +72,7 @@ WeakMapBase::markCompartmentIteratively(JSCompartment *c, JSTracer *tracer)
 {
     bool markedAny = false;
     for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next) {
-        if (m->markIteratively(tracer))
+        if (m->marked && m->markIteratively(tracer))
             markedAny = true;
     }
     return markedAny;
@@ -48,8 +81,25 @@ WeakMapBase::markCompartmentIteratively(JSCompartment *c, JSTracer *tracer)
 void
 WeakMapBase::sweepCompartment(JSCompartment *c)
 {
-    for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next)
+    WeakMapBase **tailPtr = &c->gcWeakMapList;
+    for (WeakMapBase *m = c->gcWeakMapList, *next; m; m = next) {
+        next = m->next;
+        if (m->marked) {
             m->sweep();
+            *tailPtr = m;
+            tailPtr = &m->next;
+        } else {
+            /* Destroy the hash map now to catch any use after this point. */
+            m->finish();
+            m->next = WeakMapNotInList;
+        }
+    }
+    *tailPtr = nullptr;
+
+#ifdef DEBUG
+    for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next)
+        JS_ASSERT(m->isInList() && m->marked);
+#endif
 }
 
 void
@@ -62,41 +112,24 @@ WeakMapBase::traceAllMappings(WeakMapTracer *tracer)
     }
 }
 
-void
-WeakMapBase::resetCompartmentWeakMapList(JSCompartment *c)
-{
-    JS_ASSERT(WeakMapNotInList != nullptr);
-
-    WeakMapBase *m = c->gcWeakMapList;
-    c->gcWeakMapList = nullptr;
-    while (m) {
-        WeakMapBase *n = m->next;
-        m->next = WeakMapNotInList;
-        m = n;
-    }
-}
-
 bool
-WeakMapBase::saveCompartmentWeakMapList(JSCompartment *c, WeakMapVector &vector)
+WeakMapBase::saveCompartmentMarkedWeakMaps(JSCompartment *c, WeakMapSet &markedWeakMaps)
 {
-    WeakMapBase *m = c->gcWeakMapList;
+    for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next) {
-    while (m) {
+        if (m->marked && !markedWeakMaps.put(m))
-        if (!vector.append(m))
             return false;
-        m = m->next;
     }
     return true;
 }
 
 void
-WeakMapBase::restoreCompartmentWeakMapLists(WeakMapVector &vector)
+WeakMapBase::restoreCompartmentMarkedWeakMaps(WeakMapSet &markedWeakMaps)
 {
-    for (WeakMapBase **p = vector.begin(); p != vector.end(); p++) {
+    for (WeakMapSet::Range r = markedWeakMaps.all(); !r.empty(); r.popFront()) {
-        WeakMapBase *m = *p;
+        WeakMapBase *map = r.front();
-        JS_ASSERT(m->next == WeakMapNotInList);
+        JS_ASSERT(map->compartment->zone()->isGCMarking());
-        JSCompartment *c = m->compartment;
+        JS_ASSERT(!map->marked);
-        m->next = c->gcWeakMapList;
+        map->marked = true;
-        c->gcWeakMapList = m;
     }
 }
 
@@ -166,8 +199,8 @@ WeakMap_clear_impl(JSContext *cx, CallArgs args)
 {
     JS_ASSERT(IsWeakMap(args.thisv()));
 
-    // We can't js_delete the weakmap because the data gathered during GC
+    // We can't js_delete the weakmap because the data gathered during GC is
-    // is used by the Cycle Collector
+    // used by the Cycle Collector.
     if (ObjectValueMap *map = args.thisv().toObject().as<WeakMapObject>().getMap())
         map->clear();
 
@@ -397,7 +430,6 @@ static void
 WeakMap_finalize(FreeOp *fop, JSObject *obj)
 {
     if (ObjectValueMap *map = obj->as<WeakMapObject>().getMap()) {
-        map->check();
 #ifdef DEBUG
         map->~ObjectValueMap();
         memset(static_cast<void *>(map), 0xdc, sizeof(*map));

js/src/jsweakmap.h

@@ -33,7 +33,7 @@ namespace js {
 // The value for the next pointer for maps not in the map list.
 static WeakMapBase * const WeakMapNotInList = reinterpret_cast<WeakMapBase *>(1);
 
-typedef Vector<WeakMapBase *, 0, SystemAllocPolicy> WeakMapVector;
+typedef HashSet<WeakMapBase *, DefaultHasher<WeakMapBase *>, SystemAllocPolicy> WeakMapSet;
 
 // Common base class for all WeakMap specializations. The collector uses this to call
 // their markIteratively and sweep methods.
@@ -42,62 +42,35 @@ class WeakMapBase {
     WeakMapBase(JSObject *memOf, JSCompartment *c);
     virtual ~WeakMapBase();
 
-    void trace(JSTracer *tracer) {
+    void trace(JSTracer *tracer);
-        if (IS_GC_MARKING_TRACER(tracer)) {
-            // We don't do anything with a WeakMap at trace time. Rather, we wait until as
-            // many keys as possible have been marked, and add ourselves to the list of
-            // known-live WeakMaps to be scanned in the iterative marking phase, by
-            // markAllIteratively.
-            JS_ASSERT(tracer->eagerlyTraceWeakMaps() == DoNotTraceWeakMaps);
-
-            // Add ourselves to the list if we are not already in the list. We can already
-            // be in the list if the weak map is marked more than once due delayed marking.
-            if (next == WeakMapNotInList) {
-                next = compartment->gcWeakMapList;
-                compartment->gcWeakMapList = this;
-            }
-        } else {
-            // If we're not actually doing garbage collection, the keys won't be marked
-            // nicely as needed by the true ephemeral marking algorithm --- custom tracers
-            // such as the cycle collector must use their own means for cycle detection.
-            // So here we do a conservative approximation: pretend all keys are live.
-            if (tracer->eagerlyTraceWeakMaps() == DoNotTraceWeakMaps)
-                return;
-
-            nonMarkingTraceValues(tracer);
-            if (tracer->eagerlyTraceWeakMaps() == TraceWeakMapKeysValues)
-                nonMarkingTraceKeys(tracer);
-        }
-    }
 
     // Garbage collector entry points.
 
+    // Unmark all weak maps in a compartment.
+    static void unmarkCompartment(JSCompartment *c);
+
     // Check all weak maps in a compartment that have been marked as live in this garbage
     // collection, and mark the values of all entries that have become strong references
     // to them. Return true if we marked any new values, indicating that we need to make
     // another pass. In other words, mark my marked maps' marked members' mid-collection.
     static bool markCompartmentIteratively(JSCompartment *c, JSTracer *tracer);
 
-    // Remove entries whose keys are dead from all weak maps in a compartment marked as
+    // Sweep the weak maps in a compartment, removing dead weak maps and removing
-    // live in this garbage collection.
+    // entries of live weak maps whose keys are dead.
     static void sweepCompartment(JSCompartment *c);
 
     // Trace all delayed weak map bindings. Used by the cycle collector.
     static void traceAllMappings(WeakMapTracer *tracer);
 
     bool isInList() { return next != WeakMapNotInList; }
-    void check() { JS_ASSERT(!isInList()); }
 
-    // Remove everything from the weak map list for a compartment.
+    // Save information about which weak maps are marked for a compartment.
-    static void resetCompartmentWeakMapList(JSCompartment *c);
+    static bool saveCompartmentMarkedWeakMaps(JSCompartment *c, WeakMapSet &markedWeakMaps);
 
-    // Save the live weak map list for a compartment, appending the data to a vector.
+    // Restore information about which weak maps are marked for many compartments.
-    static bool saveCompartmentWeakMapList(JSCompartment *c, WeakMapVector &vector);
+    static void restoreCompartmentMarkedWeakMaps(WeakMapSet &markedWeakMaps);
 
-    // Restore live weak map lists for multiple compartments from a vector.
+    // Remove a weakmap from its compartment's weakmaps list.
-    static void restoreCompartmentWeakMapLists(WeakMapVector &vector);
-
-    // Remove a weakmap from the live weakmaps list
     static void removeWeakMapFromList(WeakMapBase *weakmap);
 
   protected:
@@ -108,6 +81,7 @@ class WeakMapBase {
     virtual bool markIteratively(JSTracer *tracer) = 0;
     virtual void sweep() = 0;
     virtual void traceMappings(WeakMapTracer *tracer) = 0;
+    virtual void finish() = 0;
 
     // Object that this weak map is part of, if any.
     JSObject *memberOf;
@@ -115,14 +89,13 @@ class WeakMapBase {
     // Compartment that this weak map is part of.
     JSCompartment *compartment;
 
-  private:
+    // Link in a list of all WeakMaps in a compartment, headed by
-    // Link in a list of WeakMaps to mark iteratively and sweep in this garbage
+    // JSCompartment::gcWeakMapList. The last element of the list has nullptr as
-    // collection, headed by JSCompartment::gcWeakMapList. The last element of
+    // its next. Maps not in the list have WeakMapNotInList as their next.
-    // the list has nullptr as its next. Maps not in the list have
-    // WeakMapNotInList as their next.  We must distinguish these cases to
-    // avoid creating infinite lists when a weak map gets traced twice due to
-    // delayed marking.
     WeakMapBase *next;
+
+    // Whether this object has been traced during garbage collection.
+    bool marked;
 };
 
 template <class Key, class Value,
@@ -138,6 +111,15 @@ class WeakMap : public HashMap<Key, Value, HashPolicy, RuntimeAllocPolicy>, publ
     explicit WeakMap(JSContext *cx, JSObject *memOf = nullptr)
         : Base(cx->runtime()), WeakMapBase(memOf, cx->compartment()) { }
 
+    bool init(uint32_t len = 16) {
+        if (!Base::init(len))
+            return false;
+        next = compartment->gcWeakMapList;
+        compartment->gcWeakMapList = this;
+        marked = JS::IsIncrementalGCInProgress(compartment->runtimeFromMainThread());
+        return true;
+    }
+
   private:
     bool markValue(JSTracer *trc, Value *x) {
         if (gc::IsMarked(x))
@@ -216,6 +198,10 @@ class WeakMap : public HashMap<Key, Value, HashPolicy, RuntimeAllocPolicy>, publ
         assertEntriesNotAboutToBeFinalized();
     }
 
+    void finish() {
+        Base::finish();
+    }
+
     /* memberOf can be nullptr, which means that the map is not part of a JSObject. */
     void traceMappings(WeakMapTracer *tracer) {
         for (Range r = Base::all(); !r.empty(); r.popFront()) {

js/src/vm/WeakMapPtr.cpp

@@ -58,7 +58,6 @@ JS::WeakMapPtr<K, V>::destroy()
     // of known live weakmaps. If we are, remove ourselves before deleting.
     if (map->isInList())
         WeakMapBase::removeWeakMapFromList(map);
-    map->check();
     js_delete(map);
     ptr = nullptr;
 }