fixup! src: implement MemoryRetainer in Environment

joyeecheung · Mar 27, 2019 · Mar 31, 2019 · Apr 2, 2019 · Mar 31, 2019 · 81203f66b4ad962ba358e387b09a95419c7e73c1
commit 81203f66b4ad962ba358e387b09a95419c7e73c1
diff --git a/src/base_object.h b/src/base_object.h
@@ -94,7 +94,7 @@ class BaseObject : public MemoryRetainer {
   // position of members in memory are predictable. For more information please
   // refer to `doc/guides/node-postmortem-support.md`
   friend int GenDebugSymbols();
-  friend class Environment;
+  friend class CleanupHookCallback;
 
   Persistent<v8::Object> persistent_handle_;
   Environment* env_;

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -976,17 +976,17 @@ void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) {
   cleanup_hooks_.erase(search);
 }
 
-size_t Environment::CleanupHookCallback::Hash::operator()(
+size_t CleanupHookCallback::Hash::operator()(
     const CleanupHookCallback& cb) const {
   return std::hash<void*>()(cb.arg_);
 }
 
-bool Environment::CleanupHookCallback::Equal::operator()(
+bool CleanupHookCallback::Equal::operator()(
     const CleanupHookCallback& a, const CleanupHookCallback& b) const {
   return a.fn_ == b.fn_ && a.arg_ == b.arg_;
 }
 
-BaseObject* Environment::CleanupHookCallback::GetBaseObject() const {
+BaseObject* CleanupHookCallback::GetBaseObject() const {
   if (fn_ == BaseObject::DeleteMe)
     return static_cast<BaseObject*>(arg_);
   else

diff --git a/src/env.cc b/src/env.cc
@@ -123,11 +123,12 @@ void IsolateData::MemoryInfo(MemoryTracker* tracker) const {
   PER_ISOLATE_STRING_PROPERTIES(V)
 #undef V
 
-  tracker->TrackFieldWithSize(
-      "allocator", sizeof(*allocator_), "v8::ArrayBuffer::Allocator");
   if (node_allocator_ != nullptr) {
     tracker->TrackFieldWithSize(
         "node_allocator", sizeof(*node_allocator_), "NodeArrayBufferAllocator");
+  } else {
+    tracker->TrackFieldWithSize(
+        "allocator", sizeof(*allocator_), "v8::ArrayBuffer::Allocator");
   }
   tracker->TrackFieldWithSize(
       "platform", sizeof(*platform_), "MultiIsolatePlatform");
@@ -852,15 +853,25 @@ void Environment::stop_sub_worker_contexts() {
   }
 }
 
-void Environment::CleanupHookCallback::MemoryInfo(
-    MemoryTracker* tracker) const {
+void MemoryTracker::TrackField(const char* edge_name,
+                               const CleanupHookCallback& value,
+                               const char* node_name) {
+  v8::HandleScope handle_scope(isolate_);
+  // Here, we utilize the fact that CleanupHookCallback instances
+  // are all unique and won't be tracked twice in one BuildEmbedderGraph
+  // callback.
+  MemoryRetainerNode* n =
+      PushNode("CleanupHookCallback", sizeof(value), edge_name);
   // TODO(joyeecheung): at the moment only arguments of type BaseObject will be
   // identified and tracked here (based on their deleters),
   // but we may convert and track other known types here.
-  BaseObject* obj = GetBaseObject();
+  BaseObject* obj = value.GetBaseObject();
   if (obj != nullptr) {
-    tracker->TrackField("arg", obj);
+    this->TrackField("arg", obj);
   }
+  CHECK_EQ(CurrentNode(), n);
+  CHECK_NE(n->size_, 0);
+  PopNode();
 }
 
 void Environment::BuildEmbedderGraph(Isolate* isolate,

diff --git a/src/env.h b/src/env.h
@@ -739,6 +739,36 @@ class ShouldNotAbortOnUncaughtScope {
   Environment* env_;
 };
 
+class CleanupHookCallback {
+ public:
+  CleanupHookCallback(void (*fn)(void*),
+                      void* arg,
+                      uint64_t insertion_order_counter)
+      : fn_(fn), arg_(arg), insertion_order_counter_(insertion_order_counter) {}
+
+  // Only hashes `arg_`, since that is usually enough to identify the hook.
+  struct Hash {
+    inline size_t operator()(const CleanupHookCallback& cb) const;
+  };
+
+  // Compares by `fn_` and `arg_` being equal.
+  struct Equal {
+    inline bool operator()(const CleanupHookCallback& a,
+                           const CleanupHookCallback& b) const;
+  };
+
+  inline BaseObject* GetBaseObject() const;
+
+ private:
+  friend class Environment;
+  void (*fn_)(void*);
+  void* arg_;
+
+  // We keep track of the insertion order for these objects, so that we can
+  // call the callbacks in reverse order when we are cleaning up.
+  uint64_t insertion_order_counter_;
+};
+
 class Environment : public MemoryRetainer {
  public:
   Environment(const Environment&) = delete;
@@ -1224,42 +1254,6 @@ class Environment : public MemoryRetainer {
   void RunAndClearNativeImmediates();
   static void CheckImmediate(uv_check_t* handle);
 
-  class CleanupHookCallback : public MemoryRetainer {
-   public:
-    CleanupHookCallback(void (*fn)(void*),
-                        void* arg,
-                        uint64_t insertion_order_counter)
-        : fn_(fn),
-          arg_(arg),
-          insertion_order_counter_(insertion_order_counter) {}
-
-    // Only hashes `arg_`, since that is usually enough to identify the hook.
-    struct Hash {
-      inline size_t operator()(const CleanupHookCallback& cb) const;
-    };
-
-    // Compares by `fn_` and `arg_` being equal.
-    struct Equal {
-      inline bool operator()(const CleanupHookCallback& a,
-                             const CleanupHookCallback& b) const;
-    };
-
-    SET_MEMORY_INFO_NAME(CleanupHookCallback);
-    SET_SELF_SIZE(CleanupHookCallback);
-    void MemoryInfo(MemoryTracker* tracker) const override;
-
-    inline BaseObject* GetBaseObject() const;
-
-   private:
-    friend class Environment;
-    void (*fn_)(void*);
-    void* arg_;
-
-    // We keep track of the insertion order for these objects, so that we can
-    // call the callbacks in reverse order when we are cleaning up.
-    uint64_t insertion_order_counter_;
-  };
-
   // Use an unordered_set, so that we have efficient insertion and removal.
   std::unordered_set<CleanupHookCallback,
                      CleanupHookCallback::Hash,

diff --git a/src/memory_tracker.h b/src/memory_tracker.h
@@ -35,6 +35,8 @@ namespace crypto {
 class NodeBIO;
 }
 
+class CleanupHookCallback;
+
 /* Example:
  *
  * class ExampleRetainer : public MemoryRetainer {
@@ -195,6 +197,13 @@ class MemoryTracker {
   inline void TrackField(const char* edge_name,
                          const MallocedBuffer<T>& value,
                          const char* node_name = nullptr);
+  // We do not implement CleanupHookCallback as MemoryRetainer
+  // but instead specialize the method here to avoid the cost of
+  // virtual pointers.
+  // TODO(joyeecheung): do this for BaseObject and remove WrappedObject()
+  void TrackField(const char* edge_name,
+                  const CleanupHookCallback& value,
+                  const char* node_name = nullptr);
   inline void TrackField(const char* edge_name,
                          const uv_buf_t& value,
                          const char* node_name = nullptr);