Bug 1735296 - Part 2: Add an UnsafeBarePtr wrapper for use in NurseryAwareHashMap r=sfink

This is also required to remove some public GCPolicy methods for bare pointer
types.

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

Author: jonco3

js/src/gc/NurseryAwareHashMap.h

@@ -16,6 +16,29 @@
 namespace js {
 
 namespace detail {
+// A wrapper for a bare pointer, with no barriers. The only user should be for
+// NurseryAwareHashMap; it is defined externally because we need a GCPolicy for
+// its use in the contained map.
+template <typename T>
+class UnsafeBarePtr : public BarrieredBase<T> {
+ public:
+  UnsafeBarePtr() : BarrieredBase<T>(JS::SafelyInitialized<T>()) {}
+  MOZ_IMPLICIT UnsafeBarePtr(T v) : BarrieredBase<T>(v) {}
+  const T& get() const { return this->value; }
+  void set(T newValue) { this->value = newValue; }
+  DECLARE_POINTER_CONSTREF_OPS(T);
+};
+
+template <class T>
+struct UnsafeBarePtrHasher {
+  using Key = UnsafeBarePtr<T>;
+  using Lookup = T;
+
+  static HashNumber hash(const Lookup& l) { return DefaultHasher<T>::hash(l); }
+  static bool match(const Key& k, Lookup l) { return k.get() == l; }
+  static void rekey(Key& k, const Key& newKey) { k.set(newKey.get()); }
+};
+
 // This class only handles the incremental case and does not deal with nursery
 // pointers. The only users should be for NurseryAwareHashMap; it is defined
 // externally because we need a GCPolicy for its use in the contained map.
@@ -63,24 +86,23 @@ enum : bool { DuplicatesNotPossible, DuplicatesPossible };
 // hash table treats such edges strongly.
 //
 // Doing this requires some strong constraints on what can be stored in this
-// table and how it can be accessed. At the moment, this table assumes that
-// all values contain a strong reference to the key. It also requires the
-// policy to contain an |isTenured| and |needsSweep| members, which is fairly
-// non-standard. This limits its usefulness to the CrossCompartmentMap at the
-// moment, but might serve as a useful base for other tables in future.
+// table and how it can be accessed. At the moment, this table assumes that all
+// values contain a strong reference to the key. This limits its usefulness to
+// the CrossCompartmentMap at the moment, but might serve as a useful base for
+// other tables in future.
 template <typename Key, typename Value,
-          typename HashPolicy = DefaultHasher<Key>,
           typename AllocPolicy = TempAllocPolicy,
           bool AllowDuplicates = DuplicatesNotPossible>
 class NurseryAwareHashMap {
-  using BarrieredValue = detail::UnsafeBareWeakHeapPtr<Value>;
-  using MapType =
-      GCRekeyableHashMap<Key, BarrieredValue, HashPolicy, AllocPolicy>;
+  using MapKey = detail::UnsafeBarePtr<Key>;
+  using MapValue = detail::UnsafeBareWeakHeapPtr<Value>;
+  using HashPolicy = DefaultHasher<MapKey>;
+  using MapType = GCRekeyableHashMap<MapKey, MapValue, HashPolicy, AllocPolicy>;
   MapType map;
 
-  // Keep a list of all keys for which JS::GCPolicy<Key>::isTenured is false.
-  // This lets us avoid a full traveral of the map on each minor GC, keeping
-  // the minor GC times proportional to the nursery heap size.
+  // Keep a list of all keys for which key->isTenured() is false. This lets us
+  // avoid a full traversal of the map on each minor GC, keeping the minor GC
+  // times proportional to the nursery heap size.
   Vector<Key, 0, AllocPolicy> nurseryEntries;
 
  public:
@@ -111,33 +133,19 @@ class NurseryAwareHashMap {
            nurseryEntries.sizeOfIncludingThis(mallocSizeOf);
   }
 
-  [[nodiscard]] bool put(const Key& k, const Value& v) {
-    auto p = map.lookupForAdd(k);
-    if (p) {
-      if (!JS::GCPolicy<Key>::isTenured(k) ||
-          !JS::GCPolicy<Value>::isTenured(v)) {
-        if (!nurseryEntries.append(k)) {
-          return false;
-        }
-      }
-      p->value() = v;
-      return true;
-    }
-
-    bool ok = map.add(p, k, v);
-    if (!ok) {
+  [[nodiscard]] bool put(const Key& key, const Value& value) {
+    if ((!key->isTenured() || !value->isTenured()) &&
+        !nurseryEntries.append(key)) {
       return false;
     }
 
-    if (!JS::GCPolicy<Key>::isTenured(k) ||
-        !JS::GCPolicy<Value>::isTenured(v)) {
-      if (!nurseryEntries.append(k)) {
-        map.remove(k);
-        return false;
-      }
+    auto p = map.lookupForAdd(key);
+    if (p) {
+      p->value() = value;
+      return true;
     }
 
-    return true;
+    return map.add(p, key, value);
   }
 
   void sweepAfterMinorGC(JSTracer* trc) {
@@ -148,25 +156,25 @@ class NurseryAwareHashMap {
       }
 
       // Drop the entry if the value is not marked.
-      if (JS::GCPolicy<BarrieredValue>::needsSweep(&p->value())) {
+      if (JS::GCPolicy<MapValue>::needsSweep(&p->value())) {
         map.remove(key);
         continue;
       }
 
       // Update and relocate the key, if the value is still needed.
       //
-      // Non-string Values will contain a strong reference to Key, as per
-      // its use in the CrossCompartmentWrapperMap, so the key will never
-      // be dying here. Strings do *not* have any sort of pointer from
-      // wrapper to wrappee, as they are just copies. The wrapper map
-      // entry is merely used as a cache to avoid re-copying the string,
-      // and currently that entire cache is flushed on major GC.
-      Key copy(key);
-      bool sweepKey = JS::GCPolicy<Key>::needsSweep(&copy);
-      if (sweepKey) {
-        map.remove(key);
+      // Non-string Values will contain a strong reference to Key, as per its
+      // use in the CrossCompartmentWrapperMap, so the key will never be dying
+      // here. Strings do *not* have any sort of pointer from wrapper to
+      // wrappee, as they are just copies. The wrapper map entry is merely used
+      // as a cache to avoid re-copying the string, and currently that entire
+      // cache is flushed on major GC.
+      MapKey copy(key);
+      if (JS::GCPolicy<MapKey>::needsSweep(&copy)) {
+        map.remove(p);
         continue;
       }
+
       if (AllowDuplicates) {
         // Drop duplicated keys.
         //
@@ -178,7 +186,7 @@ class NurseryAwareHashMap {
         } else if (map.has(copy)) {
           // Key was forwarded to the same place that another key was already
           // forwarded to.
-          map.remove(key);
+          map.remove(p);
         } else {
           map.rekeyAs(key, copy, copy);
         }
@@ -203,6 +211,24 @@ class NurseryAwareHashMap {
 }  // namespace js
 
 namespace JS {
+
+template <typename T>
+struct GCPolicy<js::detail::UnsafeBarePtr<T>> {
+  static bool needsSweep(js::detail::UnsafeBarePtr<T>* vp) {
+    if (*vp) {
+      return js::gc::IsAboutToBeFinalizedUnbarriered(vp->unbarrieredAddress());
+    }
+    return false;
+  }
+  static bool traceWeak(JSTracer* trc, js::detail::UnsafeBarePtr<T>* vp) {
+    if (*vp) {
+      return js::TraceManuallyBarrieredWeakEdge(trc, vp->unbarrieredAddress(),
+                                                "UnsafeBarePtr");
+    }
+    return true;
+  }
+};
+
 template <typename T>
 struct GCPolicy<js::detail::UnsafeBareWeakHeapPtr<T>> {
   static void trace(JSTracer* trc, js::detail::UnsafeBareWeakHeapPtr<T>* thingp,
@@ -213,6 +239,15 @@ struct GCPolicy<js::detail::UnsafeBareWeakHeapPtr<T>> {
     return js::gc::IsAboutToBeFinalized(thingp);
   }
 };
+
 }  // namespace JS
 
+namespace mozilla {
+
+template <class T>
+struct DefaultHasher<js::detail::UnsafeBarePtr<T>>
+    : js::detail::UnsafeBarePtrHasher<T> {};
+
+}  // namespace mozilla
+
 #endif  // gc_NurseryAwareHashMap_h

js/src/gc/Zone.cpp

@@ -380,7 +380,7 @@ void Zone::checkStringWrappersAfterMovingGC() {
     // wrapper map that points into the nursery, and that the hash table entries
     // are discoverable.
     auto key = e.front().key();
-    CheckGCThingAfterMovingGC(key);
+    CheckGCThingAfterMovingGC(key.get());
 
     auto ptr = crossZoneStringWrappers().lookup(key);
     MOZ_RELEASE_ASSERT(ptr.found() && &*ptr == &e.front());

js/src/gc/Zone.h

@@ -67,8 +67,8 @@ using FinalizationRecordVector = GCVector<HeapPtrObject, 1, ZoneAllocPolicy>;
 // `DuplicatesPossible` will allow this and map both wrappers to the same (now
 // tenured) source string.
 using StringWrapperMap =
-    NurseryAwareHashMap<JSString*, JSString*, DefaultHasher<JSString*>,
-                        ZoneAllocPolicy, DuplicatesPossible>;
+    NurseryAwareHashMap<JSString*, JSString*, ZoneAllocPolicy,
+                        DuplicatesPossible>;
 
 class MOZ_NON_TEMPORARY_CLASS ExternalStringCache {
   static const size_t NumEntries = 4;

js/src/vm/Compartment.cpp

@@ -58,7 +58,7 @@ void Compartment::checkObjectWrappersAfterMovingGC() {
     // wrapper map that points into the nursery, and that the hash table entries
     // are discoverable.
     auto key = e.front().key();
-    CheckGCThingAfterMovingGC(key);
+    CheckGCThingAfterMovingGC(key.get());
 
     auto ptr = crossCompartmentObjectWrappers.lookup(key);
     MOZ_RELEASE_ASSERT(ptr.found() && &*ptr == &e.front());

js/src/vm/Compartment.h

@@ -31,9 +31,7 @@ namespace js {
 class ObjectWrapperMap {
   static const size_t InitialInnerMapSize = 4;
 
-  using InnerMap =
-      NurseryAwareHashMap<JSObject*, JSObject*, DefaultHasher<JSObject*>,
-                          ZoneAllocPolicy>;
+  using InnerMap = NurseryAwareHashMap<JSObject*, JSObject*, ZoneAllocPolicy>;
   using OuterMap = GCHashMap<JS::Compartment*, InnerMap,
                              DefaultHasher<JS::Compartment*>, ZoneAllocPolicy>;
 
@@ -247,8 +245,8 @@ class ObjectWrapperMap {
 };
 
 using StringWrapperMap =
-    NurseryAwareHashMap<JSString*, JSString*, DefaultHasher<JSString*>,
-                        ZoneAllocPolicy, DuplicatesPossible>;
+    NurseryAwareHashMap<JSString*, JSString*, ZoneAllocPolicy,
+                        DuplicatesPossible>;
 
 }  // namespace js