Use AddEdgesCallback to report WeakRefs

Summary: Edges of WeakRefs for heap snapshot can be added in AddEdgesCallback of HeapSnapshotMetadata, instead of EdgeAddingAcceptor. This is a necessary step to completely remove WeakRefAcceptor. Note that the order of edges may be different, but this is OK as Chrome Dev Tool shows it in its own order anyway. Reviewed By: neildhar Differential Revision: D51223793 fbshipit-source-id: 90fb9c74536152f88c8884df0ee3a1fb63a9f20d
facebook · Nov 16, 2023 · 4249e05 · 4249e05
1 parent 730a59f
commit 4249e05
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 34 deletions.
diff --git a/include/hermes/VM/DummyObject.h b/include/hermes/VM/DummyObject.h
@@ -58,6 +58,10 @@ struct DummyObject final : public GCCell {
   static void _finalizeImpl(GCCell *cell, GC &);
   static size_t _mallocSizeImpl(GCCell *cell);
   static void _markWeakImpl(GCCell *cell, WeakRefAcceptor &acceptor);
+
+#ifdef HERMES_MEMORY_INSTRUMENTATION
+  static void _snapshotAddEdgesImpl(GCCell *cell, GC &gc, HeapSnapshot &snap);
+#endif
 };
 
 } // namespace testhelpers

diff --git a/include/hermes/VM/JSWeakRef.h b/include/hermes/VM/JSWeakRef.h
@@ -44,6 +44,10 @@ class JSWeakRef final : public JSObject {
 
   static void _markWeakImpl(GCCell *cell, WeakRefAcceptor &acceptor);
 
+#ifdef HERMES_MEMORY_INSTRUMENTATION
+  static void _snapshotAddEdgesImpl(GCCell *cell, GC &gc, HeapSnapshot &snap);
+#endif
+
   JSWeakRef(
       Runtime &runtime,
       Handle<JSObject> parent,

diff --git a/lib/VM/DummyObject.cpp b/lib/VM/DummyObject.cpp
@@ -22,7 +22,17 @@ const VTable DummyObject::vt{
     _finalizeImpl,
     _markWeakImpl,
     _mallocSizeImpl,
-    nullptr};
+    nullptr
+#ifdef HERMES_MEMORY_INSTRUMENTATION
+    ,
+    VTable::HeapSnapshotMetadata{
+        HeapSnapshot::NodeType::Object,
+        nullptr,
+        _snapshotAddEdgesImpl,
+        nullptr,
+        nullptr}
+#endif
+};
 
 DummyObject::DummyObject(GC &gc) : other(), x(1), y(2) {
   hvBool.setNonPtr(HermesValue::encodeBoolValue(true), gc);
@@ -89,6 +99,22 @@ void DummyObject::_markWeakImpl(GCCell *cell, WeakRefAcceptor &acceptor) {
     acceptor.accept(*self->weak);
 }
 
+#ifdef HERMES_MEMORY_INSTRUMENTATION
+void DummyObject::_snapshotAddEdgesImpl(
+    GCCell *cell,
+    GC &gc,
+    HeapSnapshot &snap) {
+  auto *const self = vmcast<DummyObject>(cell);
+  if (!self->weak)
+    return;
+  // DummyObject has only one WeakRef field.
+  snap.addNamedEdge(
+      HeapSnapshot::EdgeType::Weak,
+      "weak",
+      gc.getObjectID(self->weak->getNoBarrierUnsafe(gc.getPointerBase())));
+}
+#endif
+
 } // namespace testhelpers
 
 void DummyObjectBuildMeta(const GCCell *cell, Metadata::Builder &mb) {

diff --git a/lib/VM/GCBase.cpp b/lib/VM/GCBase.cpp
@@ -255,7 +255,7 @@ struct PrimitiveNodeAcceptor : public SnapshotAcceptor {
   llvh::DenseSet<double, GCBase::IDTracker::DoubleComparator> seenNumbers_;
 };
 
-struct EdgeAddingAcceptor : public SnapshotAcceptor, public WeakRefAcceptor {
+struct EdgeAddingAcceptor : public SnapshotAcceptor {
   using SnapshotAcceptor::accept;
 
   EdgeAddingAcceptor(GCBase &gc, HeapSnapshot &snap)
@@ -285,25 +285,6 @@ struct EdgeAddingAcceptor : public SnapshotAcceptor, public WeakRefAcceptor {
     acceptHV(hv, name);
   }
 
-  void accept(WeakRefBase &wr) override {
-    WeakRefSlot *slot = wr.unsafeGetSlot();
-    if (slot->state() == WeakSlotState::Free) {
-      // If the slot is free, there's no edge to add.
-      return;
-    }
-    if (!slot->hasValue()) {
-      // Filter out empty refs from adding edges.
-      return;
-    }
-    // Assume all weak pointers have no names, and are stored in an array-like
-    // structure.
-    std::string indexName = std::to_string(nextEdge_++);
-    snap_.addNamedEdge(
-        HeapSnapshot::EdgeType::Weak,
-        indexName,
-        gc_.getObjectID(slot->getNoBarrierUnsafe(gc_.getPointerBase())));
-  }
-
   void acceptSym(SymbolID sym, const char *name) override {
     if (sym.isInvalid()) {
       return;
@@ -316,8 +297,6 @@ struct EdgeAddingAcceptor : public SnapshotAcceptor, public WeakRefAcceptor {
 
  private:
   GCBase &gc_;
-  // For unnamed edges, use indices instead.
-  unsigned nextEdge_{0};
 };
 
 struct SnapshotRootSectionAcceptor : public SnapshotAcceptor,

diff --git a/lib/VM/HiddenClass.cpp b/lib/VM/HiddenClass.cpp
@@ -47,6 +47,17 @@ void TransitionMap::snapshotAddNodes(GC &gc, HeapSnapshot &snap) {
 }
 
 void TransitionMap::snapshotAddEdges(GC &gc, HeapSnapshot &snap) {
+  uint32_t edge_index = 0;
+  forEachEntry([&snap, &gc, &edge_index](
+                   const Transition &, const WeakRef<HiddenClass> &value) {
+    if (value.isValid()) {
+      snap.addNamedEdge(
+          HeapSnapshot::EdgeType::Weak,
+          std::to_string(edge_index++),
+          gc.getObjectID(value.getNoBarrierUnsafe(gc.getPointerBase())));
+    }
+  });
+
   if (!isLarge()) {
     return;
   }

diff --git a/lib/VM/JSWeakMapImpl.cpp b/lib/VM/JSWeakMapImpl.cpp
@@ -213,6 +213,16 @@ void JSWeakMapImplBase::_snapshotAddEdgesImpl(
     snap.addNamedEdge(
         HeapSnapshot::EdgeType::Internal, "map", self->getMapID(gc));
   }
+
+  // Add edges to objects pointed by WeakRef keys.
+  uint32_t edge_index = 0;
+  for (const auto &[key, _] : self->map_) {
+    auto indexName = std::to_string(edge_index++);
+    snap.addNamedEdge(
+        HeapSnapshot::EdgeType::Weak,
+        indexName,
+        gc.getObjectID(key.ref.getNoBarrierUnsafe(gc.getPointerBase())));
+  }
 }
 
 void JSWeakMapImplBase::_snapshotAddNodesImpl(

diff --git a/lib/VM/JSWeakRef.cpp b/lib/VM/JSWeakRef.cpp
@@ -18,7 +18,17 @@ const ObjectVTable JSWeakRef::vt{
         CellKind::JSWeakRefKind,
         cellSize<JSWeakRef>(),
         nullptr,
-        JSWeakRef::_markWeakImpl),
+        JSWeakRef::_markWeakImpl,
+        nullptr,
+        nullptr
+#ifdef HERMES_MEMORY_INSTRUMENTATION
+        ,
+        VTable::HeapSnapshotMetadata {
+          HeapSnapshot::NodeType::Object, nullptr,
+              JSWeakRef::_snapshotAddEdgesImpl, nullptr, nullptr
+        }
+#endif
+        ),
     JSWeakRef::_getOwnIndexedRangeImpl,
     JSWeakRef::_haveOwnIndexedImpl,
     JSWeakRef::_getOwnIndexedPropertyFlagsImpl,
@@ -41,6 +51,19 @@ void JSWeakRef::_markWeakImpl(GCCell *cell, WeakRefAcceptor &acceptor) {
   }
 }
 
+#ifdef HERMES_MEMORY_INSTRUMENTATION
+void JSWeakRef::_snapshotAddEdgesImpl(
+    GCCell *cell,
+    GC &gc,
+    HeapSnapshot &snap) {
+  auto *const self = vmcast<JSWeakRef>(cell);
+  snap.addNamedEdge(
+      HeapSnapshot::EdgeType::Weak,
+      "weak",
+      gc.getObjectID(self->ref_.getNoBarrierUnsafe(gc.getPointerBase())));
+}
+#endif
+
 PseudoHandle<JSWeakRef> JSWeakRef::create(
     Runtime &runtime,
     Handle<JSObject> parentHandle) {

diff --git a/unittests/VMRuntime/HeapSnapshotTest.cpp b/unittests/VMRuntime/HeapSnapshotTest.cpp
@@ -668,7 +668,7 @@ TEST(HeapSnapshotTest, TestNodesAndEdgesForDummyObjects) {
               numberEdge,
               undefinedEdge,
               nullEdge,
-              Edge{HeapSnapshot::EdgeType::Weak, "0", firstDummy.id}}));
+              Edge{HeapSnapshot::EdgeType::Weak, "weak", firstDummy.id}}));
 
   EXPECT_EQ(
       FIND_NODE_AND_EDGES_FOR_ID(secondDummy.id, nodes, edges, strings),
@@ -679,7 +679,7 @@ TEST(HeapSnapshotTest, TestNodesAndEdgesForDummyObjects) {
               numberEdge,
               undefinedEdge,
               nullEdge,
-              Edge{HeapSnapshot::EdgeType::Weak, "0", secondDummy.id}}));
+              Edge{HeapSnapshot::EdgeType::Weak, "weak", secondDummy.id}}));
 }
 
 TEST(HeapSnapshotTest, SnapshotFromCallbackContext) {
@@ -885,17 +885,10 @@ TEST_F(HeapSnapshotRuntimeTest, WeakMapTest) {
           firstNamed + 3));
   EXPECT_EQ(nodesAndEdges.second.size(), firstNamed + 3);
 
-  // Test the weak edge.
-  EXPECT_EQ(
-      nodesAndEdges.second[firstNamed],
-      Edge(
-          HeapSnapshot::EdgeType::Weak,
-          "0",
-          runtime.getHeap().getObjectID(key.get())));
   // Test the native edge.
   const auto nativeMapID = map->getMapID(runtime.getHeap());
   EXPECT_EQ(
-      nodesAndEdges.second[firstNamed + 2],
+      nodesAndEdges.second[firstNamed + 1],
       Edge(HeapSnapshot::EdgeType::Internal, "map", nativeMapID));
   EXPECT_EQ(
       FIND_NODE_FOR_ID(nativeMapID, nodes, strings),
@@ -905,6 +898,14 @@ TEST_F(HeapSnapshotRuntimeTest, WeakMapTest) {
           nativeMapID,
           map->getMallocSize(),
           0));
+
+  // Test the weak edge.
+  EXPECT_EQ(
+      nodesAndEdges.second[firstNamed + 2],
+      Edge(
+          HeapSnapshot::EdgeType::Weak,
+          "0",
+          runtime.getHeap().getObjectID(key.get())));
 }
 
 TEST_F(HeapSnapshotRuntimeTest, PropertyUpdatesTest) {