Bug 1463462 - Make weak map marking take account of the fact that black and gray marking can now be interleaved r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 06 Dec 2018 16:27:22 -0500
changeset 507866 1aefa97ae11619e27931c3fbacb8ce777ed98720
parent 507865 6ae14f44b4af53cb2ecf23eb5077ab90d1615ea9
child 507867 e3fc6ddd9a5316edac3737f92666e3cd0c08a44e
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1463462
milestone66.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 1463462 - Make weak map marking take account of the fact that black and gray marking can now be interleaved r=sfink
js/src/gc/GCMarker.h
js/src/gc/Marking.cpp
js/src/gc/Marking.h
js/src/gc/Verifier.cpp
js/src/gc/WeakMap-inl.h
js/src/gc/WeakMap.cpp
js/src/gc/WeakMap.h
--- a/js/src/gc/GCMarker.h
+++ b/js/src/gc/GCMarker.h
@@ -266,16 +266,31 @@ class GCMarker : public JSTracer {
    * objects. If this invariant is violated, the cycle collector may free
    * objects that are still reachable.
    */
   void setMarkColorGray();
   void setMarkColorBlack();
   void setMarkColor(gc::MarkColor newColor);
   gc::MarkColor markColor() const { return color; }
 
+  // Return whether a cell is marked relative to the current marking color. If
+  // the cell is black then this returns true, but if it's gray it will return
+  // false if the mark color is black.
+  template <typename T>
+  bool isMarked(T* thingp) {
+    return color == gc::MarkColor::Black ? gc::IsMarkedBlack(runtime(), thingp)
+                                         : gc::IsMarked(runtime(), thingp);
+  }
+  template <typename T>
+  bool isMarkedUnbarriered(T* thingp) {
+    return color == gc::MarkColor::Black
+               ? gc::IsMarkedBlackUnbarriered(runtime(), thingp)
+               : gc::IsMarkedUnbarriered(runtime(), thingp);
+  }
+
   void enterWeakMarkingMode();
   void leaveWeakMarkingMode();
   void abortLinearWeakMarking() {
     leaveWeakMarkingMode();
     linearWeakMarkingDisabled_ = true;
   }
 
   void delayMarkingArena(gc::Arena* arena);
--- a/js/src/gc/Marking.cpp
+++ b/js/src/gc/Marking.cpp
@@ -611,19 +611,25 @@ struct ImplicitEdgeHolderType<JSObject*>
 
 template <>
 struct ImplicitEdgeHolderType<JSScript*> {
   typedef JSScript* Type;
 };
 
 void GCMarker::markEphemeronValues(gc::Cell* markedCell,
                                    WeakEntryVector& values) {
-  size_t initialLen = values.length();
-  for (size_t i = 0; i < initialLen; i++) {
-    values[i].weakmap->markEntry(this, markedCell, values[i].key);
+  DebugOnly<size_t> initialLen = values.length();
+
+  for (const auto& markable : values) {
+    if (color == gc::MarkColor::Black &&
+        markable.weakmap->markColor == gc::MarkColor::Gray) {
+      continue;
+    }
+
+    markable.weakmap->markEntry(this, markedCell, markable.key);
   }
 
   // The vector should not be appended to during iteration because the key is
   // already marked, and even in cases where we have a multipart key, we
   // should only be inserting entries for the unmarked portions.
   MOZ_ASSERT(values.length() == initialLen);
 }
 
@@ -3236,32 +3242,32 @@ static inline void CheckIsMarkedThing(T*
       !ThingIsPermanentAtomOrWellKnownSymbol(*thingp),
       CurrentThreadCanAccessRuntime(rt) ||
           CurrentThreadCanAccessZone((*thingp)->zoneFromAnyThread()) ||
           (JS::RuntimeHeapIsCollecting() && rt->gc.state() == State::Sweep));
 #endif
 }
 
 template <typename T>
-static bool IsMarkedInternalCommon(T* thingp) {
+static inline bool ShouldCheckMarkState(JSRuntime* rt, T** thingp) {
   CheckIsMarkedThing(thingp);
   MOZ_ASSERT(!IsInsideNursery(*thingp));
 
   TenuredCell& thing = (*thingp)->asTenured();
   Zone* zone = thing.zoneFromAnyThread();
   if (!zone->isCollectingFromAnyThread() || zone->isGCFinished()) {
-    return true;
+    return false;
   }
 
   if (zone->isGCCompacting() && IsForwarded(*thingp)) {
     *thingp = Forwarded(*thingp);
-    return true;
+    return false;
   }
 
-  return thing.isMarkedAny();
+  return true;
 }
 
 template <typename T>
 struct MightBeNurseryAllocated {
   static const bool value = mozilla::IsBaseOf<JSObject, T>::value ||
                             mozilla::IsBaseOf<JSString, T>::value;
 };
 
@@ -3272,17 +3278,40 @@ bool js::gc::IsMarkedInternal(JSRuntime*
   }
 
   if (MightBeNurseryAllocated<T>::value && IsInsideNursery(*thingp)) {
     MOZ_ASSERT(CurrentThreadCanAccessRuntime(rt));
     Cell** cellp = reinterpret_cast<Cell**>(thingp);
     return Nursery::getForwardedPointer(cellp);
   }
 
-  return IsMarkedInternalCommon(thingp);
+  if (!ShouldCheckMarkState(rt, thingp)) {
+    return true;
+  }
+
+  return (*thingp)->asTenured().isMarkedAny();
+}
+
+template <typename T>
+bool js::gc::IsMarkedBlackInternal(JSRuntime* rt, T** thingp) {
+  if (IsOwnedByOtherRuntime(rt, *thingp)) {
+    return true;
+  }
+
+  if (MightBeNurseryAllocated<T>::value && IsInsideNursery(*thingp)) {
+    MOZ_ASSERT(CurrentThreadCanAccessRuntime(rt));
+    Cell** cellp = reinterpret_cast<Cell**>(thingp);
+    return Nursery::getForwardedPointer(cellp);
+  }
+
+  if (!ShouldCheckMarkState(rt, thingp)) {
+    return true;
+  }
+
+  return (*thingp)->asTenured().isMarkedBlack();
 }
 
 template <typename S>
 struct IsMarkedFunctor : public IdentityDefaultAdaptor<S> {
   template <typename T>
   S operator()(T* t, JSRuntime* rt, bool* rv) {
     *rv = IsMarkedInternal(rt, &t);
     return js::gc::RewrapTaggedPointer<S, T>::wrap(t);
@@ -3291,16 +3320,32 @@ struct IsMarkedFunctor : public Identity
 
 template <typename T>
 bool js::gc::IsMarkedInternal(JSRuntime* rt, T* thingp) {
   bool rv = true;
   *thingp = DispatchTyped(IsMarkedFunctor<T>(), *thingp, rt, &rv);
   return rv;
 }
 
+template <typename S>
+struct IsMarkedBlackFunctor : public IdentityDefaultAdaptor<S> {
+  template <typename T>
+  S operator()(T* t, JSRuntime* rt, bool* rv) {
+    *rv = IsMarkedBlackInternal(rt, &t);
+    return js::gc::RewrapTaggedPointer<S, T>::wrap(t);
+  }
+};
+
+template <typename T>
+bool js::gc::IsMarkedBlackInternal(JSRuntime* rt, T* thingp) {
+  bool rv = true;
+  *thingp = DispatchTyped(IsMarkedBlackFunctor<T>(), *thingp, rt, &rv);
+  return rv;
+}
+
 bool js::gc::IsAboutToBeFinalizedDuringSweep(TenuredCell& tenured) {
   MOZ_ASSERT(!IsInsideNursery(&tenured));
   MOZ_ASSERT(tenured.zoneFromAnyThread()->isGCSweeping());
   return !tenured.isMarkedAny();
 }
 
 template <typename T>
 bool js::gc::IsAboutToBeFinalizedInternal(T** thingp) {
@@ -3363,18 +3408,19 @@ JS_PUBLIC_API bool EdgeNeedsSweepUnbarri
 // Instantiate a copy of the Tracing templates for each public GC type.
 #define INSTANTIATE_ALL_VALID_HEAP_TRACE_FUNCTIONS(type)             \
   template JS_PUBLIC_API bool EdgeNeedsSweep<type>(JS::Heap<type>*); \
   template JS_PUBLIC_API bool EdgeNeedsSweepUnbarrieredSlow<type>(type*);
 FOR_EACH_PUBLIC_GC_POINTER_TYPE(INSTANTIATE_ALL_VALID_HEAP_TRACE_FUNCTIONS)
 FOR_EACH_PUBLIC_TAGGED_GC_POINTER_TYPE(
     INSTANTIATE_ALL_VALID_HEAP_TRACE_FUNCTIONS)
 
-#define INSTANTIATE_INTERNAL_MARKING_FUNCTIONS(type)          \
-  template bool IsMarkedInternal(JSRuntime* rt, type* thing); \
+#define INSTANTIATE_INTERNAL_MARKING_FUNCTIONS(type)               \
+  template bool IsMarkedInternal(JSRuntime* rt, type* thing);      \
+  template bool IsMarkedBlackInternal(JSRuntime* rt, type* thing); \
   template bool IsAboutToBeFinalizedInternal(type* thingp);
 
 #define INSTANTIATE_INTERNAL_MARKING_FUNCTIONS_FROM_TRACEKIND(_1, type, _2) \
   INSTANTIATE_INTERNAL_MARKING_FUNCTIONS(type*)
 
 JS_FOR_EACH_TRACEKIND(INSTANTIATE_INTERNAL_MARKING_FUNCTIONS_FROM_TRACEKIND)
 FOR_EACH_PUBLIC_TAGGED_GC_POINTER_TYPE(INSTANTIATE_INTERNAL_MARKING_FUNCTIONS)
 
--- a/js/src/gc/Marking.h
+++ b/js/src/gc/Marking.h
@@ -63,37 +63,55 @@ void PushArena(GCMarker* gcmarker, Arena
 // separate implementations.
 
 template <typename T>
 bool IsMarkedInternal(JSRuntime* rt, T* thing);
 template <typename T>
 bool IsMarkedInternal(JSRuntime* rt, T** thing);
 
 template <typename T>
+bool IsMarkedBlackInternal(JSRuntime* rt, T* thing);
+template <typename T>
+bool IsMarkedBlackInternal(JSRuntime* rt, T** thing);
+
+template <typename T>
 bool IsAboutToBeFinalizedInternal(T* thingp);
 template <typename T>
 bool IsAboutToBeFinalizedInternal(T** thingp);
 
-// Report whether a thing has been marked.  Things which are in zones that are
-// not currently being collected or are owned by another runtime are always
-// reported as being marked.
+// Report whether a GC thing has been marked with any color. Things which are in
+// zones that are not currently being collected or are owned by another runtime
+// are always reported as being marked.
 template <typename T>
 inline bool IsMarkedUnbarriered(JSRuntime* rt, T* thingp) {
   return IsMarkedInternal(rt, ConvertToBase(thingp));
 }
 
-// Report whether a thing has been marked.  Things which are in zones that are
-// not currently being collected or are owned by another runtime are always
-// reported as being marked.
+// Report whether a GC thing has been marked with any color. Things which are in
+// zones that are not currently being collected or are owned by another runtime
+// are always reported as being marked.
 template <typename T>
 inline bool IsMarked(JSRuntime* rt, WriteBarrieredBase<T>* thingp) {
   return IsMarkedInternal(rt,
                           ConvertToBase(thingp->unsafeUnbarrieredForTracing()));
 }
 
+// Report whether a GC thing has been marked black.
+template <typename T>
+inline bool IsMarkedBlackUnbarriered(JSRuntime* rt, T* thingp) {
+  return IsMarkedBlackInternal(rt, ConvertToBase(thingp));
+}
+
+// Report whether a GC thing has been marked black.
+template <typename T>
+inline bool IsMarkedBlack(JSRuntime* rt, WriteBarrieredBase<T>* thingp) {
+  return IsMarkedBlackInternal(
+      rt, ConvertToBase(thingp->unsafeUnbarrieredForTracing()));
+}
+
 template <typename T>
 inline bool IsAboutToBeFinalizedUnbarriered(T* thingp) {
   return IsAboutToBeFinalizedInternal(ConvertToBase(thingp));
 }
 
 template <typename T>
 inline bool IsAboutToBeFinalized(WriteBarrieredBase<T>* thingp) {
   return IsAboutToBeFinalizedInternal(
--- a/js/src/gc/Verifier.cpp
+++ b/js/src/gc/Verifier.cpp
@@ -773,19 +773,19 @@ bool js::gc::CheckWeakMapEntryMarking(co
   // Debugger weak maps can have keys in different zones.
   Zone* keyZone = GetCellZone(key);
   MOZ_ASSERT_IF(!map->allowKeysInOtherZones(),
                 keyZone == zone || keyZone->isAtomsZone());
 
   Zone* valueZone = GetCellZone(value);
   MOZ_ASSERT(valueZone == zone || valueZone->isAtomsZone());
 
-  // We may not know the color of the map, but we know that it's
-  // alive so it must at least be marked gray.
-  CellColor mapColor = object ? GetCellColor(object) : CellColor::Gray;
+  CellColor mapColor = map->markColor == MarkColor::Black ? CellColor::Black
+                                                          : CellColor::Gray;
+  MOZ_ASSERT_IF(object, GetCellColor(object) == mapColor);
 
   CellColor keyColor = GetCellColor(key);
   CellColor valueColor = valueZone->isGCMarking() ? GetCellColor(value)
                                                   : CellColor::Black;
 
   if (valueColor < Min(mapColor, keyColor)) {
     fprintf(stderr, "WeakMap value is less marked than map and key\n");
     fprintf(stderr, "(map %p is %s, key %p is %s, value %p is %s)\n", map,
--- a/js/src/gc/WeakMap-inl.h
+++ b/js/src/gc/WeakMap-inl.h
@@ -29,17 +29,20 @@ WeakMap<K, V>::WeakMap(JSContext* cx, JS
   using ElemType = typename K::ElementType;
   using NonPtrType = typename mozilla::RemovePointer<ElemType>::Type;
   // The object's TraceKind needs to be added to CC graph if this object is
   // used as a WeakMap key. See the comments for IsCCTraceKind for details.
   static_assert(JS::IsCCTraceKind(NonPtrType::TraceKind),
                 "Object's TraceKind should be added to CC graph.");
 
   zone()->gcWeakMapList().insertFront(this);
-  marked = JS::IsIncrementalGCInProgress(TlsContext.get());
+  if (zone()->wasGCStarted()) {
+    marked = true;
+    markColor = gc::MarkColor::Black;
+  }
 }
 
 // Trace a WeakMap entry based on 'markedCell' getting marked, where 'origKey'
 // is the key in the weakmap. These will probably be the same, but can be
 // different eg when markedCell is a delegate for origKey.
 //
 // This implementation does not use 'markedCell'; it looks up origKey and checks
 // the mark bits on everything it cares about, one of which will be
@@ -53,36 +56,38 @@ void WeakMap<K, V>::markEntry(GCMarker* 
   // Lookup that can't be constructed from a Cell*. The WeakKeyTable
   // mechanism is indexed with a GCCellPtr, so that won't work.
   Ptr p = Base::lookup(static_cast<Lookup>(origKey.asCell()));
   MOZ_ASSERT(p.found());
 
   K key(p->key());
   MOZ_ASSERT((markedCell == extractUnbarriered(key)) ||
              (markedCell == getDelegate(key)));
-  if (gc::IsMarked(marker->runtime(), &key)) {
+  if (marker->isMarked(&key)) {
     TraceEdge(marker, &p->value(), "ephemeron value");
-  } else if (keyNeedsMark(key)) {
+  } else if (keyNeedsMark(marker, key)) {
     TraceEdge(marker, &p->value(), "WeakMap ephemeron value");
     TraceEdge(marker, &key, "proxy-preserved WeakMap ephemeron key");
     MOZ_ASSERT(key == p->key());  // No moving
   }
   key.unsafeSet(nullptr);  // Prevent destructor from running barriers.
 }
 
 template <class K, class V>
 void WeakMap<K, V>::trace(JSTracer* trc) {
   MOZ_ASSERT_IF(JS::RuntimeHeapIsBusy(), isInList());
 
   TraceNullableEdge(trc, &memberOf, "WeakMap owner");
 
   if (trc->isMarkingTracer()) {
     MOZ_ASSERT(trc->weakMapAction() == ExpandWeakMaps);
+    auto marker = GCMarker::fromTracer(trc);
     marked = true;
-    (void)markIteratively(GCMarker::fromTracer(trc));
+    markColor = marker->markColor();
+    (void)markIteratively(marker);
     return;
   }
 
   if (trc->weakMapAction() == DoNotTraceWeakMaps) {
     return;
   }
 
   // Trace keys only if weakMapAction() says to.
@@ -117,31 +122,35 @@ template <class K, class V>
       marker->abortLinearWeakMarking();
     }
   }
 }
 
 template <class K, class V>
 bool WeakMap<K, V>::markIteratively(GCMarker* marker) {
   MOZ_ASSERT(marked);
+  if (marker->markColor() == gc::MarkColor::Black &&
+      markColor == gc::MarkColor::Gray) {
+    return false;
+  }
 
   bool markedAny = false;
 
   for (Enum e(*this); !e.empty(); e.popFront()) {
     // If the entry is live, ensure its key and value are marked.
-    bool keyIsMarked = gc::IsMarked(marker->runtime(), &e.front().mutableKey());
-    if (!keyIsMarked && keyNeedsMark(e.front().key())) {
+    bool keyIsMarked = marker->isMarked(&e.front().mutableKey());
+    if (!keyIsMarked && keyNeedsMark(marker, e.front().key())) {
       TraceEdge(marker, &e.front().mutableKey(),
                 "proxy-preserved WeakMap entry key");
       keyIsMarked = true;
       markedAny = true;
     }
 
     if (keyIsMarked) {
-      if (!gc::IsMarked(marker->runtime(), &e.front().value())) {
+      if (!marker->isMarked(&e.front().value())) {
         TraceEdge(marker, &e.front().value(), "WeakMap entry value");
         markedAny = true;
       }
     } else if (marker->isWeakMarkingTracer()) {
       // Entry is not yet known to be live. Record this weakmap and
       // the lookup key in the list of weak keys. Also record the
       // delegate, if any, because marking the delegate also marks
       // the entry.
@@ -181,33 +190,34 @@ inline JSObject* WeakMap<K, V>::getDeleg
 }
 
 template <class K, class V>
 inline JSObject* WeakMap<K, V>::getDelegate(LazyScript* script) const {
   return nullptr;
 }
 
 template <class K, class V>
-inline bool WeakMap<K, V>::keyNeedsMark(JSObject* key) const {
+inline bool WeakMap<K, V>::keyNeedsMark(GCMarker* marker, JSObject* key) const {
   JSObject* delegate = getDelegate(key);
   /*
    * Check if the delegate is marked with any color to properly handle
    * gray marking when the key's delegate is black and the map is gray.
    */
-  return delegate &&
-         gc::IsMarkedUnbarriered(zone()->runtimeFromMainThread(), &delegate);
+  return delegate && marker->isMarkedUnbarriered(&delegate);
 }
 
 template <class K, class V>
-inline bool WeakMap<K, V>::keyNeedsMark(JSScript* script) const {
+inline bool WeakMap<K, V>::keyNeedsMark(GCMarker* marker,
+                                        JSScript* script) const {
   return false;
 }
 
 template <class K, class V>
-inline bool WeakMap<K, V>::keyNeedsMark(LazyScript* script) const {
+inline bool WeakMap<K, V>::keyNeedsMark(GCMarker* marker,
+                                        LazyScript* script) const {
   return false;
 }
 
 template <class K, class V>
 void WeakMap<K, V>::sweep() {
   /* Remove all entries whose keys remain unmarked. */
   for (Enum e(*this); !e.empty(); e.popFront()) {
     if (gc::IsAboutToBeFinalized(&e.front().mutableKey())) {
--- a/js/src/gc/WeakMap.cpp
+++ b/js/src/gc/WeakMap.cpp
@@ -18,17 +18,17 @@
 #include "vm/JSObject.h"
 
 #include "vm/JSObject-inl.h"
 
 using namespace js;
 using namespace js::gc;
 
 WeakMapBase::WeakMapBase(JSObject* memOf, Zone* zone)
-    : memberOf(memOf), zone_(zone), marked(false) {
+    : memberOf(memOf), zone_(zone), marked(false), markColor(MarkColor::Black) {
   MOZ_ASSERT_IF(memberOf, memberOf->compartment()->zone() == zone);
 }
 
 WeakMapBase::~WeakMapBase() {
   MOZ_ASSERT(CurrentThreadIsGCSweeping() || CurrentThreadCanAccessZone(zone_));
 }
 
 void WeakMapBase::unmarkZone(JS::Zone* zone) {
--- a/js/src/gc/WeakMap.h
+++ b/js/src/gc/WeakMap.h
@@ -124,18 +124,20 @@ class WeakMapBase : public mozilla::Link
 #endif
 
   // Object that this weak map is part of, if any.
   GCPtrObject memberOf;
 
   // Zone containing this weak map.
   JS::Zone* zone_;
 
-  // Whether this object has been traced during garbage collection.
+  // Whether this object has been marked during garbage collection and which
+  // color it was marked.
   bool marked;
+  gc::MarkColor markColor;
 };
 
 template <class Key, class Value>
 class WeakMap
     : public HashMap<Key, Value, MovableCellHasher<Key>, ZoneAllocPolicy>,
       public WeakMapBase {
  public:
   typedef HashMap<Key, Value, MovableCellHasher<Key>, ZoneAllocPolicy> Base;
@@ -188,19 +190,19 @@ class WeakMap
  private:
   void exposeGCThingToActiveJS(const JS::Value& v) const {
     JS::ExposeValueToActiveJS(v);
   }
   void exposeGCThingToActiveJS(JSObject* obj) const {
     JS::ExposeObjectToActiveJS(obj);
   }
 
-  bool keyNeedsMark(JSObject* key) const;
-  bool keyNeedsMark(JSScript* script) const;
-  bool keyNeedsMark(LazyScript* script) const;
+  bool keyNeedsMark(GCMarker* marker, JSObject* key) const;
+  bool keyNeedsMark(GCMarker* marker, JSScript* script) const;
+  bool keyNeedsMark(GCMarker* marker, LazyScript* script) const;
 
   bool findZoneEdges() override {
     // This is overridden by ObjectValueMap.
     return true;
   }
 
   void sweep() override;