Bug 1573809 - Part 6 : GCHashmap/GCHashTable traceWeak. r=jonco

Differential Revision: https://phabricator.services.mozilla.com/D46614

--HG--
extra : moz-landing-system : lando

js/public/GCHashTable.h

@@ -23,6 +23,11 @@ struct DefaultMapSweepPolicy {
   static bool needsSweep(Key* key, Value* value) {
     return GCPolicy<Key>::needsSweep(key) || GCPolicy<Value>::needsSweep(value);
   }
+
+  static bool traceWeak(JSTracer* trc, Key* key, Value* value) {
+    return GCPolicy<Key>::traceWeak(trc, key) &&
+           GCPolicy<Value>::traceWeak(trc, value);
+  }
 };
 
 // A GCHashMap is a GC-aware HashMap, meaning that it has additional trace and
@@ -79,6 +84,15 @@ class GCHashMap : public js::HashMap<Key, Value, HashPolicy, AllocPolicy> {
     }
   }
 
+  void traceWeak(JSTracer* trc) {
+    for (typename Base::Enum e(*this); !e.empty(); e.popFront()) {
+      if (!MapSweepPolicy::traceWeak(trc, &e.front().mutableKey(),
+                                     &e.front().value())) {
+        e.removeFront();
+      }
+    }
+  }
+
   // GCHashMap is movable
   GCHashMap(GCHashMap&& rhs) : Base(std::move(rhs)) {}
   void operator=(GCHashMap&& rhs) {
@@ -127,6 +141,17 @@ class GCRekeyableHashMap : public JS::GCHashMap<Key, Value, HashPolicy,
     }
   }
 
+  void traceWeak(JSTracer* trc) {
+    for (typename Base::Enum e(*this); !e.empty(); e.popFront()) {
+      Key key(e.front().key());
+      if (!MapSweepPolicy::traceWeak(trc, &key, &e.front().value())) {
+        e.removeFront();
+      } else if (!HashPolicy::match(key, e.front().key())) {
+        e.rekeyFront(key);
+      }
+    }
+  }
+
   // GCRekeyableHashMap is movable
   GCRekeyableHashMap(GCRekeyableHashMap&& rhs) : Base(std::move(rhs)) {}
   void operator=(GCRekeyableHashMap&& rhs) {
@@ -252,6 +277,14 @@ class GCHashSet : public js::HashSet<T, HashPolicy, AllocPolicy> {
     }
   }
 
+  void traceWeak(JSTracer* trc) {
+    for (typename Base::Enum e(*this); !e.empty(); e.popFront()) {
+      if (!GCPolicy<T>::traceWeak(trc, &e.mutableFront())) {
+        e.removeFront();
+      }
+    }
+  }
+
   // GCHashSet is movable
   GCHashSet(GCHashSet&& rhs) : Base(std::move(rhs)) {}
   void operator=(GCHashSet&& rhs) {

js/public/GCPolicyAPI.h

@@ -18,20 +18,29 @@
 //         GCHashMap and GCHashSet use this method to trace their children.
 //
 //   static bool needsSweep(T* tp)
-//       - Return true if |*tp| is about to be finalized. Otherwise, update the
+//       - [DEPRECATED], use traceWeak instead.
+//         Return true if |*tp| is about to be finalized. Otherwise, update the
 //         edge for moving GC, and return false. Containers like GCHashMap and
 //         GCHashSet use this method to decide when to remove an entry: if this
 //         function returns true on a key/value/member/etc, its entry is dropped
 //         from the container. Specializing this method is the standard way to
 //         get custom weak behavior from a container type.
 //
+//   static bool traceWeak(T* tp)
+//       - Return false if |*tp| has been set to nullptr. Otherwise, update the
+//         edge for moving GC, and return true. Containers like GCHashMap and
+//         GCHashSet use this method to decide when to remove an entry: if this
+//         function returns false on a key/value/member/etc, its entry is
+//         dropped from the container. Specializing this method is the standard
+//         way to get custom weak behavior from a container type.
+//
 //   static bool isValid(const T& t)
 //       - Return false only if |t| is corrupt in some way. The built-in GC
 //         types do some memory layout checks. For debugging only; it is ok
 //         to always return true or even to omit this member entirely.
 //
 // The default GCPolicy<T> assumes that T has a default constructor and |trace|
-// and |needsSweep| methods, and forwards to them. GCPolicy has appropriate
+// and |traceWeak| methods, and forwards to them. GCPolicy has appropriate
 // specializations for pointers to GC things and pointer-like types like
 // JS::Heap<T> and mozilla::UniquePtr<T>.
 //
@@ -81,6 +90,8 @@ struct StructGCPolicy {
 
   static bool needsSweep(T* tp) { return tp->needsSweep(); }
 
+  static bool traceWeak(JSTracer* trc, T* tp) { return tp->traceWeak(trc); }
+
   static bool isValid(const T& tp) { return true; }
 };
 
@@ -96,6 +107,7 @@ template <typename T>
 struct IgnoreGCPolicy {
   static void trace(JSTracer* trc, T* t, const char* name) {}
   static bool needsSweep(T* v) { return false; }
+  static bool traceWeak(JSTracer*, T* v) { return true; }
   static bool isValid(const T& v) { return true; }
 };
 template <>
@@ -119,6 +131,12 @@ struct GCPointerPolicy {
     }
     return false;
   }
+  static bool traceWeak(JSTracer* trc, T* vp) {
+    if (*vp) {
+      return js::TraceManuallyBarrieredWeakEdge(trc, vp, "traceWeak");
+    }
+    return true;
+  }
   static bool isTenured(T v) { return !js::gc::IsInsideNursery(v); }
   static bool isValid(T v) { return js::gc::IsCellPointerValidOrNull(v); }
 };
@@ -143,6 +161,13 @@ struct NonGCPointerPolicy {
     }
     return false;
   }
+  static bool traceWeak(JSTracer* trc, T* vp) {
+    if (*vp) {
+      return (*vp)->traceWeak(trc);
+    }
+    return true;
+  }
+
   static bool isValid(T v) { return true; }
 };
 
@@ -154,6 +179,12 @@ struct GCPolicy<JS::Heap<T>> {
   static bool needsSweep(JS::Heap<T>* thingp) {
     return *thingp && js::gc::EdgeNeedsSweep(thingp);
   }
+  static bool traceWeak(JSTracer* trc, JS::Heap<T>* thingp) {
+    if (*thingp) {
+      return js::TraceWeakEdge(trc, thingp, "traceWeak");
+    }
+    return true;
+  }
 };
 
 // GCPolicy<UniquePtr<T>> forwards the contained pointer to GCPolicy<T>.
@@ -171,6 +202,12 @@ struct GCPolicy<mozilla::UniquePtr<T, D>> {
     }
     return false;
   }
+  static bool traceWeak(JSTracer* trc, mozilla::UniquePtr<T, D>* tp) {
+    if (tp->get()) {
+      return GCPolicy<T>::traceWeak(trc, tp->get());
+    }
+    return true;
+  }
   static bool isValid(const mozilla::UniquePtr<T, D>& t) {
     if (t.get()) {
       return GCPolicy<T>::isValid(*t.get());
@@ -194,6 +231,12 @@ struct GCPolicy<mozilla::Maybe<T>> {
     }
     return false;
   }
+  static bool traceWeak(JSTracer* trc, mozilla::Maybe<T>* tp) {
+    if (tp->isSome()) {
+      return GCPolicy<T>::traceWeak(trc, tp->ptr());
+    }
+    return true;
+  }
   static bool isValid(const mozilla::Maybe<T>& t) {
     if (t.isSome()) {
       return GCPolicy<T>::isValid(t.ref());

js/public/TracingAPI.h

@@ -468,6 +468,19 @@ extern JS_PUBLIC_API void UnsafeTraceManuallyBarrieredEdge(JSTracer* trc,
                                                            T* edgep,
                                                            const char* name);
 
+// Not part of the public API, but declared here so we can use it in
+// GCPolicyAPI.h
+template <typename T>
+inline bool TraceManuallyBarrieredWeakEdge(JSTracer* trc, T* thingp,
+                                           const char* name);
+
+template <typename T>
+class BarrieredBase;
+
+template <typename T>
+inline bool TraceWeakEdge(JSTracer* trc, BarrieredBase<T>* thingp,
+                          const char* name);
+
 namespace gc {
 
 // Return true if the given edge is not live and is about to be swept.

js/src/gc/GC.cpp

@@ -2093,13 +2093,13 @@ void GCRuntime::sweepZoneAfterCompacting(MovingTracer* trc, Zone* zone) {
   }
 
   if (jit::JitZone* jitZone = zone->jitZone()) {
-    jitZone->sweep();
+    jitZone->traceWeak(trc);
   }
 
   for (RealmsInZoneIter r(zone); !r.done(); r.next()) {
-    r->sweepObjectGroups();
+    r->traceWeakObjectGroups(trc);
     r->traceWeakRegExps(trc);
-    r->sweepSavedStacks();
+    r->traceWeakSavedStacks(trc);
     r->sweepVarNames();
     r->traceWeakObjects(trc);
     r->traceWeakSelfHostingScriptSource(trc);
@@ -5095,9 +5095,10 @@ static void SweepCCWrappers(GCParallelTask* task) {
 }
 
 static void SweepObjectGroups(GCParallelTask* task) {
+  SweepingTracer trc(task->gc->rt);
   for (SweepGroupRealmsIter r(task->gc); !r.done(); r.next()) {
     AutoSetThreadIsSweeping threadIsSweeping(r->zone());
-    r->sweepObjectGroups();
+    r->traceWeakObjectGroups(&trc);
   }
 }
 
@@ -5107,7 +5108,7 @@ static void SweepMisc(GCParallelTask* task) {
     AutoSetThreadIsSweeping threadIsSweeping(r->zone());
     r->traceWeakObjects(&trc);
     r->traceWeakTemplateObjects(&trc);
-    r->sweepSavedStacks();
+    r->traceWeakSavedStacks(&trc);
     r->traceWeakSelfHostingScriptSource(&trc);
     r->traceWeakObjectRealm(&trc);
     r->traceWeakRegExps(&trc);
@@ -5245,7 +5246,7 @@ void GCRuntime::sweepJitDataOnMainThread(JSFreeOp* fop) {
 
     for (SweepGroupZonesIter zone(this); !zone.done(); zone.next()) {
       if (jit::JitZone* jitZone = zone->jitZone()) {
-        jitZone->sweep();
+        jitZone->traceWeak(&trc);
       }
     }
   }

js/src/gc/GCInternals.h

@@ -188,6 +188,7 @@ struct SweepingTracer final : public JS::CallbackTracer {
   bool onScopeEdge(Scope** scopep) override;
   bool onRegExpSharedEdge(RegExpShared** sharedp) override;
   bool onBigIntEdge(BigInt** bip) override;
+  bool onObjectGroupEdge(js::ObjectGroup** groupp) override;
   bool onChild(const JS::GCCellPtr& thing) override {
     MOZ_CRASH("unexpected edge.");
     return true;

js/src/gc/Marking.cpp

@@ -3499,6 +3499,9 @@ bool SweepingTracer::onScopeEdge(Scope** scopep) { return sweepEdge(scopep); }
 bool SweepingTracer::onRegExpSharedEdge(RegExpShared** sharedp) {
   return sweepEdge(sharedp);
 }
+bool SweepingTracer::onObjectGroupEdge(ObjectGroup** groupp) {
+  return sweepEdge(groupp);
+}
 bool SweepingTracer::onBigIntEdge(BigInt** bip) { return sweepEdge(bip); }
 
 namespace js {

js/src/gc/Policy.h

@@ -70,6 +70,9 @@ struct GCPolicy<js::HeapPtr<T>> {
   static bool needsSweep(js::HeapPtr<T>* thingp) {
     return js::gc::IsAboutToBeFinalized(thingp);
   }
+  static bool traceWeak(JSTracer* trc, js::HeapPtr<T>* thingp) {
+    return js::TraceWeakEdge(trc, thingp, "traceWeak");
+  }
 };
 
 template <typename T>
@@ -81,6 +84,9 @@ struct GCPolicy<js::WeakHeapPtr<T>> {
   static bool needsSweep(js::WeakHeapPtr<T>* thingp) {
     return js::gc::IsAboutToBeFinalized(thingp);
   }
+  static bool traceWeak(JSTracer* trc, js::WeakHeapPtr<T>* thingp) {
+    return js::TraceWeakEdge(trc, thingp, "traceWeak");
+  }
 };
 
 }  // namespace JS

js/src/jit/Ion.cpp

@@ -584,7 +584,7 @@ void JitRealm::traceWeak(JSTracer* trc, JS::Realm* realm) {
   // Any outstanding compilations should have been cancelled by the GC.
   MOZ_ASSERT(!HasOffThreadIonCompile(realm));
 
-  stubCodes_->sweep();
+  stubCodes_->traceWeak(trc);
 
   for (WeakHeapPtrJitCode& stub : stubs_) {
     if (stub) {
@@ -593,7 +593,9 @@ void JitRealm::traceWeak(JSTracer* trc, JS::Realm* realm) {
   }
 }
 
-void JitZone::sweep() { baselineCacheIRStubCodes_.sweep(); }
+void JitZone::traceWeak(JSTracer* trc) {
+  baselineCacheIRStubCodes_.traceWeak(trc);
+}
 
 size_t JitRealm::sizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const {
   size_t n = mallocSizeOf(this);

js/src/jit/JitRealm.h

@@ -478,8 +478,8 @@ struct CacheIRStubKey : public DefaultHasher<CacheIRStubKey> {
 
 template <typename Key>
 struct IcStubCodeMapGCPolicy {
-  static bool needsSweep(Key*, WeakHeapPtrJitCode* value) {
-    return IsAboutToBeFinalized(value);
+  static bool traceWeak(JSTracer* trc, Key*, WeakHeapPtrJitCode* value) {
+    return TraceWeakEdge(trc, value, "traceWeak");
   }
 };
 
@@ -501,7 +501,7 @@ class JitZone {
   BaselineCacheIRStubCodeMap baselineCacheIRStubCodes_;
 
  public:
-  void sweep();
+  void traceWeak(JSTracer* trc);
 
   void addSizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf,
                               size_t* jitZone, size_t* baselineStubsOptimized,

js/src/vm/ObjectGroup.cpp

@@ -779,18 +779,18 @@ struct ObjectGroupRealm::ArrayObjectKey : public DefaultHasher<ArrayObjectKey> {
 
   bool operator!=(const ArrayObjectKey& other) { return !(*this == other); }
 
-  bool needsSweep() {
+  bool traceWeak(JSTracer* trc) {
     MOZ_ASSERT(type.isUnknown() || !type.isSingleton());
     if (!type.isUnknown() && type.isGroup()) {
       ObjectGroup* group = type.groupNoBarrier();
-      if (IsAboutToBeFinalizedUnbarriered(&group)) {
-        return true;
+      if (!TraceManuallyBarrieredWeakEdge(trc, &group, "ObjectGroup")) {
+        return false;
       }
       if (group != type.groupNoBarrier()) {
         type = TypeSet::ObjectType(group);
       }
     }
-    return false;
+    return true;
   }
 };
 
@@ -1060,13 +1060,14 @@ struct ObjectGroupRealm::PlainObjectKey {
     return true;
   }
 
-  bool needsSweep() {
+  bool traceWeak(JSTracer* trc) {
     for (unsigned i = 0; i < nproperties; i++) {
-      if (gc::IsAboutToBeFinalizedUnbarriered(&properties[i])) {
-        return true;
+      if (!TraceManuallyBarrieredWeakEdge(trc, &properties[i],
+                                          "PlainObjectKey::properties")) {
+        return false;
       }
     }
-    return false;
+    return true;
   }
 };
 
@@ -1075,26 +1076,27 @@ struct ObjectGroupRealm::PlainObjectEntry {
   WeakHeapPtrShape shape;
   TypeSet::Type* types;
 
-  bool needsSweep(unsigned nproperties) {
-    if (IsAboutToBeFinalized(&group)) {
-      return true;
+  bool traceWeak(JSTracer* trc, unsigned nproperties) {
+    if (!TraceWeakEdge(trc, &group, "PlainObjectEntry::group")) {
+      return false;
     }
-    if (IsAboutToBeFinalized(&shape)) {
-      return true;
+    if (!TraceWeakEdge(trc, &shape, "PlainObjectEntry::shape")) {
+      return false;
     }
     for (unsigned i = 0; i < nproperties; i++) {
       MOZ_ASSERT(!types[i].isSingleton());
       if (types[i].isGroup()) {
         ObjectGroup* group = types[i].groupNoBarrier();
-        if (IsAboutToBeFinalizedUnbarriered(&group)) {
-          return true;
+        if (!TraceManuallyBarrieredWeakEdge(trc, &group,
+                                            "PlainObjectEntry::types::group")) {
+          return false;
         }
         if (group != types[i].groupNoBarrier()) {
           types[i] = TypeSet::ObjectType(group);
         }
       }
     }
-    return false;
+    return true;
   }
 };
 
@@ -1783,34 +1785,33 @@ void ObjectGroupRealm::clearTables() {
 }
 
 /* static */
-bool ObjectGroupRealm::PlainObjectTableSweepPolicy::needsSweep(
-    PlainObjectKey* key, PlainObjectEntry* entry) {
-  if (!(JS::GCPolicy<PlainObjectKey>::needsSweep(key) ||
-        entry->needsSweep(key->nproperties))) {
-    return false;
+bool ObjectGroupRealm::PlainObjectTableSweepPolicy::traceWeak(
+    JSTracer* trc, PlainObjectKey* key, PlainObjectEntry* entry) {
+  if (JS::GCPolicy<PlainObjectKey>::traceWeak(trc, key) &&
+      entry->traceWeak(trc, key->nproperties)) {
+    return true;
   }
+
   js_free(key->properties);
   js_free(entry->types);
-  return true;
+  return false;
 }
 
-void ObjectGroupRealm::sweep() {
+void ObjectGroupRealm::traceWeak(JSTracer* trc) {
   /*
    * Iterate through the array/object group tables and remove all entries
    * referencing collected data. These tables only hold weak references.
    */
 
   if (arrayObjectTable) {
-    arrayObjectTable->sweep();
+    arrayObjectTable->traceWeak(trc);
   }
   if (plainObjectTable) {
-    plainObjectTable->sweep();
+    plainObjectTable->traceWeak(trc);
   }
   if (stringSplitStringGroup) {
-    if (JS::GCPolicy<WeakHeapPtrObjectGroup>::needsSweep(
-            &stringSplitStringGroup)) {
-      stringSplitStringGroup = nullptr;
-    }
+    JS::GCPolicy<WeakHeapPtrObjectGroup>::traceWeak(trc,
+                                                    &stringSplitStringGroup);
   }
 }
 

js/src/vm/ObjectGroup.h

@@ -545,7 +545,8 @@ class ObjectGroupRealm {
   struct PlainObjectKey;
   struct PlainObjectEntry;
   struct PlainObjectTableSweepPolicy {
-    static bool needsSweep(PlainObjectKey* key, PlainObjectEntry* entry);
+    static bool traceWeak(JSTracer* trc, PlainObjectKey* key,
+                          PlainObjectEntry* entry);
   };
   using PlainObjectTable =
       JS::GCHashMap<PlainObjectKey, PlainObjectEntry, PlainObjectKey,
@@ -642,7 +643,7 @@ class ObjectGroupRealm {
 
   void clearTables();
 
-  void sweep();
+  void traceWeak(JSTracer* trc);
 
   void purge() { defaultNewGroupCache.purge(); }
 

js/src/vm/Realm.cpp

@@ -366,7 +366,7 @@ void Realm::sweepAfterMinorGC() {
   objects_.sweepAfterMinorGC();
 }
 
-void Realm::sweepSavedStacks() { savedStacks_.sweep(); }
+void Realm::traceWeakSavedStacks(JSTracer* trc) { savedStacks_.traceWeak(trc); }
 
 void Realm::traceWeakObjects(JSTracer* trc) {
   if (global_) {

js/src/vm/Realm.h

@@ -547,7 +547,7 @@ class JS::Realm : public JS::shadow::Realm {
   void traceWeakSelfHostingScriptSource(JSTracer* trc);
   void traceWeakTemplateObjects(JSTracer* trc);
 
-  void sweepObjectGroups() { objectGroups_.sweep(); }
+  void traceWeakObjectGroups(JSTracer* trc) { objectGroups_.traceWeak(trc); }
 
   void clearScriptCounts();
   void clearScriptLCov();
@@ -798,7 +798,7 @@ class JS::Realm : public JS::shadow::Realm {
     savedStacks_.chooseSamplingProbability(this);
   }
 
-  void sweepSavedStacks();
+  void traceWeakSavedStacks(JSTracer* trc);
 
   static constexpr size_t offsetOfCompartment() {
     return offsetof(JS::Realm, compartment_);

js/src/vm/SavedStacks.cpp

@@ -1322,9 +1322,9 @@ bool SavedStacks::copyAsyncStack(JSContext* cx, HandleObject asyncStack,
   return true;
 }
 
-void SavedStacks::sweep() {
-  frames.sweep();
-  pcLocationMap.sweep();
+void SavedStacks::traceWeak(JSTracer* trc) {
+  frames.traceWeak(trc);
+  pcLocationMap.traceWeak(trc);
 }
 
 void SavedStacks::trace(JSTracer* trc) { pcLocationMap.trace(trc); }

js/src/vm/SavedStacks.h

@@ -168,7 +168,7 @@ class SavedStacks {
                                    HandleString asyncCause,
                                    MutableHandleSavedFrame adoptedStack,
                                    const mozilla::Maybe<size_t>& maxFrameCount);
-  void sweep();
+  void traceWeak(JSTracer* trc);
   void trace(JSTracer* trc);
   uint32_t count();
   void clear();
@@ -239,7 +239,9 @@ class SavedStacks {
 
     void trace(JSTracer* trc) { /* PCKey is weak. */
     }
-    bool needsSweep() { return IsAboutToBeFinalized(&script); }
+    bool traceWeak(JSTracer* trc) {
+      return TraceWeakEdge(trc, &script, "traceWeak");
+    }
   };
 
  public:
@@ -253,13 +255,11 @@ class SavedStacks {
       TraceNullableEdge(trc, &source, "SavedStacks::LocationValue::source");
     }
 
-    bool needsSweep() {
-      // LocationValue is always held strongly, but in a weak map.
-      // Assert that it has been marked already, but allow it to be
-      // ejected from the map when the key dies.
+    bool traceWeak(JSTracer* trc) {
       MOZ_ASSERT(source);
-      MOZ_ASSERT(!IsAboutToBeFinalized(&source));
-      return true;
+      // TODO: Bug 1501334: IsAboutToBeFinalized doesn't work for atoms.
+      // Otherwise we should assert TraceWeakEdge always returns true;
+      return TraceWeakEdge(trc, &source, "traceWeak");
     }
 
     HeapPtr<JSAtom*> source;