Bug 1548242 - Use cross compartment wrapper map to calculate debugger's sweep group edges where possible r=sfink?
authorJon Coppeard <jcoppeard@mozilla.com>
Fri, 03 May 2019 16:49:13 +0000
changeset 472622 7637b7b1a6d24f9b172665929b7e026e0a3a3dc7
parent 472621 267df62ca84ea961b13ab7cf1531c29a8dfdcf42
child 472623 7df5932c9da0928f3cc0ece509293e5aeb98864b
push id35969
push userccoroiu@mozilla.com
push dateMon, 06 May 2019 04:24:23 +0000
treeherdermozilla-central@f0566d1a9ab1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1548242
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1548242 - Use cross compartment wrapper map to calculate debugger's sweep group edges where possible r=sfink? Rather than consulting the debugger weak maps to add reverse edges corresponding to forward edges added for cross compartment wrappers, add both forward and reverse edges when iterating the cross compartment wrapper map. Remove zone counts from debugger weak maps. Differential Revision: https://phabricator.services.mozilla.com/D29496
js/src/gc/GC.cpp
js/src/vm/Compartment.h
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -4905,35 +4905,44 @@ static void DropStringWrappers(JSRuntime
  * any strongly-connected component of this graph must start sweeping in the
  * same slice.
  *
  * Tarjan's algorithm is used to calculate the components.
  */
 
 bool Compartment::findSweepGroupEdges() {
   Zone* source = zone();
-  for (js::WrapperMap::Enum e(crossCompartmentWrappers); !e.empty();
-       e.popFront()) {
+  for (WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront()) {
     CrossCompartmentKey& key = e.front().mutableKey();
     MOZ_ASSERT(!key.is<JSString*>());
 
-    // Add edge to wrapped object zone if wrapped object is not marked black to
-    // ensure that wrapper zone not finish marking after we start sweeping the
-    // wrapped zone.
-
+    Zone* target = key.zone();
+    if (!target->isGCMarking()) {
+      continue;
+    }
+
+    // Ensure that debuggers and their debuggees are finalized in the same group
+    // by adding edges in both directions if we find a debugger wrapper.
+    // (Additional edges are added by Debugger::findSweepGroupEdges.)
+    if (key.isDebuggerKey()) {
+      if (!source->addSweepGroupEdgeTo(target) ||
+          !target->addSweepGroupEdgeTo(source)) {
+        return false;
+      }
+      continue;
+    }
+
+    // Otherwise add an edge to the wrapped object's zone to ensure that the
+    // wrapper zone is not still being marked when we start sweeping the wrapped
+    // zone.
+
+    // As an optimization, if the wrapped object is already marked black there
+    // is no danger of later marking and we can skip this.
     if (key.is<JSObject*>() &&
         key.as<JSObject*>()->asTenured().isMarkedBlack()) {
-      // CCW target is already marked, so we don't need to watch out for
-      // later marking of the CCW.
-      continue;
-    }
-
-    Zone* target =
-        key.applyToWrapped([](auto tp) { return (*tp)->asTenured().zone(); });
-    if (!target->isGCMarking()) {
       continue;
     }
 
     if (!source->addSweepGroupEdgeTo(target)) {
       return false;
     }
   }
 
@@ -4948,27 +4957,27 @@ bool Zone::findSweepGroupEdges(Zone* ato
   }
 
   for (CompartmentsInZoneIter comp(this); !comp.done(); comp.next()) {
     if (!comp->findSweepGroupEdges()) {
       return false;
     }
   }
 
-  return WeakMapBase::findSweepGroupEdges(this) &&
-         Debugger::findSweepGroupEdges(this);
+  return WeakMapBase::findSweepGroupEdges(this);
 }
 
 bool GCRuntime::findSweepGroupEdges() {
   for (GCZonesIter zone(rt); !zone.done(); zone.next()) {
     if (!zone->findSweepGroupEdges(atomsZone)) {
       return false;
     }
   }
-  return true;
+
+  return Debugger::findSweepGroupEdges(rt);
 }
 
 void GCRuntime::groupZonesForSweeping(JS::GCReason reason) {
 #ifdef DEBUG
   for (ZonesIter zone(rt, WithAtoms); !zone.done(); zone.next()) {
     MOZ_ASSERT(zone->gcSweepGroupEdges().empty());
   }
 #endif
--- a/js/src/vm/Compartment.h
+++ b/js/src/vm/Compartment.h
@@ -118,20 +118,41 @@ class CrossCompartmentKey {
       }
       ReturnType operator()(DebuggerAndObject& tpl) {
         return f_(&mozilla::Get<0>(tpl));
       }
     } matcher(f);
     return wrapped.match(matcher);
   }
 
+  bool isDebuggerKey() const {
+    struct DebuggerMatcher {
+      bool operator()(JSObject* const& obj) { return false; }
+      bool operator()(JSString* const& str) { return false; }
+      bool operator()(const DebuggerAndScript& tpl) {
+        return true;
+      }
+      bool operator()(const DebuggerAndLazyScript& tpl) {
+        return true;
+      }
+      bool operator()(const DebuggerAndObject& tpl) {
+        return true;
+      }
+    } matcher;
+    return wrapped.match(matcher);
+  }
+
   JS::Compartment* compartment() {
     return applyToWrapped([](auto tp) { return (*tp)->maybeCompartment(); });
   }
 
+  JS::Zone* zone() {
+    return applyToWrapped([](auto tp) { return (*tp)->zone(); });
+  }
+
   struct Hasher : public DefaultHasher<CrossCompartmentKey> {
     struct HashFunctor {
       HashNumber operator()(JSObject* obj) {
         return DefaultHasher<JSObject*>::hash(obj);
       }
       HashNumber operator()(JSString* str) {
         return DefaultHasher<JSString*>::hash(str);
       }
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -3539,54 +3539,41 @@ void Debugger::detachAllDebuggersFromGlo
   const GlobalObject::DebuggerVector* debuggers = global->getDebuggers();
   MOZ_ASSERT(!debuggers->empty());
   while (!debuggers->empty()) {
     debuggers->back()->removeDebuggeeGlobal(fop, global, nullptr);
   }
 }
 
 /* static */
-bool Debugger::findSweepGroupEdges(Zone* zone) {
-  JSRuntime* rt = zone->runtimeFromMainThread();
+bool Debugger::findSweepGroupEdges(JSRuntime* rt) {
+  // Ensure that debuggers and their debuggees are finalized in the same group
+  // by adding edges in both directions for debuggee zones. These are weak
+  // references that are not in the cross compartment wrapper map.
+
   for (Debugger* dbg : rt->debuggerList()) {
     Zone* debuggerZone = dbg->object->zone();
     if (!debuggerZone->isGCMarking()) {
       continue;
     }
 
-    if (debuggerZone == zone) {
-      // Add edges to debuggee zones. These are weak references that are
-      // not in the cross compartment wrapper map.
-      for (auto e = dbg->debuggeeZones.all(); !e.empty(); e.popFront()) {
-        Zone* debuggeeZone = e.front();
-        if (debuggeeZone->isGCMarking()) {
-          if (!zone->addSweepGroupEdgeTo(debuggeeZone)) {
-            return false;
-          }
-        }
-      }
-    } else {
-      // For debugger cross compartment wrappers, add edges in the
-      // opposite direction to those already added by
-      // Compartment::findOutgoingEdges and above. This ensure that
-      // debuggers and their debuggees are finalized in the same group.
-      if (dbg->debuggeeZones.has(zone) ||
-          dbg->generatorFrames.hasKeyInZone(zone) ||
-          dbg->scripts.hasKeyInZone(zone) ||
-          dbg->lazyScripts.hasKeyInZone(zone) ||
-          dbg->sources.hasKeyInZone(zone) || dbg->objects.hasKeyInZone(zone) ||
-          dbg->environments.hasKeyInZone(zone) ||
-          dbg->wasmInstanceScripts.hasKeyInZone(zone) ||
-          dbg->wasmInstanceSources.hasKeyInZone(zone)) {
-        if (!zone->addSweepGroupEdgeTo(debuggerZone)) {
+    for (auto e = dbg->debuggeeZones.all(); !e.empty(); e.popFront()) {
+      Zone* debuggeeZone = e.front();
+      if (!debuggeeZone->isGCMarking()) {
+        continue;
+      }
+
+
+      if (!debuggerZone->addSweepGroupEdgeTo(debuggeeZone) ||
+          !debuggeeZone->addSweepGroupEdgeTo(debuggerZone)) {
           return false;
-        }
-      }
-    }
-  }
+      }
+    }
+  }
+
   return true;
 }
 
 const ClassOps Debugger::classOps_ = {nullptr, /* addProperty */
                                       nullptr, /* delProperty */
                                       nullptr, /* enumerate   */
                                       nullptr, /* newEnumerate */
                                       nullptr, /* resolve     */
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -131,79 +131,61 @@ class DebuggerWeakMap
  private:
   typedef HeapPtr<UnbarrieredKey> Key;
   typedef HeapPtr<JSObject*> Value;
 
   typedef HashMap<JS::Zone*, uintptr_t, DefaultHasher<JS::Zone*>,
                   ZoneAllocPolicy>
       CountMap;
 
-  CountMap zoneCounts;
   JS::Compartment* compartment;
 
  public:
   typedef WeakMap<Key, Value> Base;
 
   explicit DebuggerWeakMap(JSContext* cx)
-      : Base(cx), zoneCounts(cx->zone()), compartment(cx->compartment()) {}
+      : Base(cx), compartment(cx->compartment()) {}
 
  public:
   // Expose those parts of HashMap public interface that are used by Debugger
   // methods.
 
   using Entry = typename Base::Entry;
   using Ptr = typename Base::Ptr;
   using AddPtr = typename Base::AddPtr;
   using Range = typename Base::Range;
   using Enum = typename Base::Enum;
   using Lookup = typename Base::Lookup;
 
   // Expose WeakMap public interface.
 
   using Base::all;
+  using Base::has;
   using Base::lookup;
   using Base::lookupForAdd;
   using Base::remove;
   using Base::trace;
 
   template <typename KeyInput, typename ValueInput>
   bool relookupOrAdd(AddPtr& p, const KeyInput& k, const ValueInput& v) {
     MOZ_ASSERT(v->compartment() == this->compartment);
 #ifdef DEBUG
     CheckDebuggeeThing(k, InvisibleKeysOk);
 #endif
     MOZ_ASSERT(!Base::has(k));
-    if (!incZoneCount(k->zone())) {
-      return false;
-    }
     bool ok = Base::relookupOrAdd(p, k, v);
-    if (!ok) {
-      decZoneCount(k->zone());
-    }
     return ok;
   }
 
-  template <typename KeyInput>
-  bool has(const KeyInput& k) const {
-    return !!this->lookup(k);
-  }
-
-  void remove(const Lookup& l) {
-    MOZ_ASSERT(Base::has(l));
-    Base::remove(l);
-    decZoneCount(l->zone());
-  }
-
   // Remove entries whose keys satisfy the given predicate.
   template <typename Predicate>
   void removeIf(Predicate test) {
     for (Enum e(*static_cast<Base*>(this)); !e.empty(); e.popFront()) {
       JSObject* key = e.front().key();
       if (test(key)) {
-        decZoneCount(key->zoneFromAnyThread());
         e.removeFront();
       }
     }
   }
 
  public:
   template <void(traceValueEdges)(JSTracer*, JSObject*)>
   void traceCrossCompartmentEdges(JSTracer* tracer) {
@@ -213,56 +195,17 @@ class DebuggerWeakMap
       TraceEdge(tracer, &key, "Debugger WeakMap key");
       if (key != e.front().key()) {
         e.rekeyFront(key);
       }
       key.unsafeSet(nullptr);
     }
   }
 
-  bool hasKeyInZone(JS::Zone* zone) {
-    CountMap::Ptr p = zoneCounts.lookup(zone);
-    MOZ_ASSERT_IF(p.found(), p->value() > 0);
-    return p.found();
-  }
-
  private:
-  /* Override sweep method to also update our edge cache. */
-  void sweep() override {
-    MOZ_ASSERT(CurrentThreadIsPerformingGC());
-    for (Enum e(*static_cast<Base*>(this)); !e.empty(); e.popFront()) {
-      if (gc::IsAboutToBeFinalized(&e.front().mutableKey())) {
-        decZoneCount(e.front().key()->zoneFromAnyThread());
-        e.removeFront();
-      }
-    }
-#ifdef DEBUG
-    Base::assertEntriesNotAboutToBeFinalized();
-#endif
-  }
-
-  MOZ_MUST_USE bool incZoneCount(JS::Zone* zone) {
-    CountMap::AddPtr p = zoneCounts.lookupForAdd(zone);
-    if (!p && !zoneCounts.add(p, zone, 0)) {
-      return false;  // OOM'd while adding
-    }
-    ++p->value();
-    return true;
-  }
-
-  void decZoneCount(JS::Zone* zone) {
-    CountMap::Ptr p = zoneCounts.lookup(zone);
-    MOZ_ASSERT(p);
-    MOZ_ASSERT(p->value() > 0);
-    --p->value();
-    if (p->value() == 0) {
-      zoneCounts.remove(zone);
-    }
-  }
-
 #ifdef JS_GC_ZEAL
   // Let the weak map marking verifier know that this map can
   // contain keys in other zones.
   virtual bool allowKeysInOtherZones() const override { return true; }
 #endif
 };
 
 class LeaveDebuggeeNoExecute;
@@ -986,17 +929,17 @@ class Debugger : private mozilla::Linked
    * objects that are definitely live but not yet marked, it marks them and
    * returns true. If not, it returns false.
    */
   static void traceIncomingCrossCompartmentEdges(JSTracer* tracer);
   static MOZ_MUST_USE bool markIteratively(GCMarker* marker);
   static void traceAllForMovingGC(JSTracer* trc);
   static void sweepAll(FreeOp* fop);
   static void detachAllDebuggersFromGlobal(FreeOp* fop, GlobalObject* global);
-  static MOZ_MUST_USE bool findSweepGroupEdges(JS::Zone* zone);
+  static MOZ_MUST_USE bool findSweepGroupEdges(JSRuntime* rt);
 #ifdef DEBUG
   static bool isDebuggerCrossCompartmentEdge(JSObject* obj,
                                              const js::gc::Cell* cell);
 #endif
 
   // Checks it the current compartment is allowed to execute code.
   static inline MOZ_MUST_USE bool checkNoExecute(JSContext* cx,
                                                  HandleScript script);