Do not reuse PersistentContextPtr instances

bindings/profilers/wall.cc

@@ -105,89 +105,35 @@ void SetContextPtr(ContextPtr& contextPtr,
   }
 }
 
-class PersistentContextPtr {
+class PersistentContextPtr : public node::ObjectWrap {
   ContextPtr context;
-  WallProfiler* owner;
-  Persistent<Object> per;
+  std::unordered_set<PersistentContextPtr*>* live;
 
  public:
-  PersistentContextPtr(WallProfiler* owner) : owner(owner) {}
-
-  void UnregisterFromGC() {
-    if (!per.IsEmpty()) {
-      per.ClearWeak();
-      per.Reset();
-    }
+  PersistentContextPtr(std::unordered_set<PersistentContextPtr*>* live,
+                       Local<Object> wrap)
+      : live(live) {
+    Wrap(wrap);
   }
 
-  void MarkDead() {
-    context.reset();
-    owner->MarkDeadPersistentContextPtr(this);
-  }
+  void Detach() { live = nullptr; }
 
-  void RegisterForGC(Isolate* isolate, const Local<Object>& obj) {
-    // Register a callback to delete this object when the object is GCed
-    per.Reset(isolate, obj);
-    per.SetWeak(
-        this,
-        [](const WeakCallbackInfo<PersistentContextPtr>& data) {
-          auto ptr = data.GetParameter();
-          ptr->UnregisterFromGC();
-          ptr->MarkDead();
-        },
-        WeakCallbackType::kParameter);
+  ~PersistentContextPtr() {
+    if (live) {
+      live->erase(this);
+    }
   }
 
   void Set(Isolate* isolate, const Local<Value>& value) {
     SetContextPtr(context, isolate, value);
   }
 
   ContextPtr Get() const { return context; }
-};
-
-void WallProfiler::MarkDeadPersistentContextPtr(PersistentContextPtr* ptr) {
-  deadContextPtrs_.push_back(ptr);
-  liveContextPtrs_.erase(ptr);
-  // Cap freelist growth by a dynamic byte budget based on live async contexts.
-  constexpr size_t kMinDeadContextPtrBudgetBytes = 512 * 1024;        // 512 KiB
-  constexpr size_t kMaxDeadContextPtrBudgetBytes = 16 * 1024 * 1024;  // 16 MiB
-  constexpr size_t kDeadContextPtrMultiplier = 2;
-  const size_t perPtrBytes = sizeof(PersistentContextPtr);
-  size_t maxDeadContextPtrs = kMaxDeadContextPtrBudgetBytes / perPtrBytes;
-  size_t minDeadContextPtrs = kMinDeadContextPtrBudgetBytes / perPtrBytes;
-  if (minDeadContextPtrs > maxDeadContextPtrs) {
-    minDeadContextPtrs = maxDeadContextPtrs;
-  }
-
-  const size_t liveCount = liveContextPtrs_.size();
-  size_t targetDeadContextPtrs;
-  if (liveCount >= maxDeadContextPtrs / kDeadContextPtrMultiplier) {
-    targetDeadContextPtrs = maxDeadContextPtrs;
-  } else {
-    targetDeadContextPtrs = liveCount * kDeadContextPtrMultiplier;
-    if (targetDeadContextPtrs < minDeadContextPtrs) {
-      targetDeadContextPtrs = minDeadContextPtrs;
-    }
-  }
 
-  const size_t shrinkThreshold =
-      targetDeadContextPtrs + targetDeadContextPtrs / 2;  // 1.5x hysteresis
-  if (deadContextPtrs_.size() <= shrinkThreshold) {
-    return;
-  }
-
-  const size_t emergencyThreshold = maxDeadContextPtrs * 2;  // 2x max
-  size_t toTrim = deadContextPtrs_.size() - targetDeadContextPtrs;
-  if (deadContextPtrs_.size() <= emergencyThreshold && toTrim > trimBatch_) {
-    toTrim = trimBatch_;
+  static PersistentContextPtr* Unwrap(Local<Object> wrap) {
+    return node::ObjectWrap::Unwrap<PersistentContextPtr>(wrap);
   }
-  while (toTrim > 0) {
-    auto* toDelete = deadContextPtrs_.front();
-    deadContextPtrs_.pop_front();
-    delete toDelete;
-    --toTrim;
-  }
-}
+};
 
 // Maximum number of rounds in the GetV8ToEpochOffset
 static constexpr int MAX_EPOCH_OFFSET_ATTEMPTS = 20;
@@ -859,6 +805,15 @@ WallProfiler::WallProfiler(std::chrono::microseconds samplingPeriod,
 #endif  // DD_WALL_USE_CPED
 }
 
+WallProfiler::~WallProfiler() {
+  // Delete all live contexts
+  for (auto ptr : liveContextPtrs_) {
+    ptr->Detach();  // so it doesn't invalidate our iterator
+    delete ptr;
+  }
+  liveContextPtrs_.clear();
+}
+
 void WallProfiler::Dispose(Isolate* isolate, bool removeFromMap) {
   if (cpuProfiler_ != nullptr) {
     cpuProfiler_->Dispose();
@@ -875,19 +830,6 @@ void WallProfiler::Dispose(Isolate* isolate, bool removeFromMap) {
 
     node::RemoveEnvironmentCleanupHook(
         isolate, &WallProfiler::CleanupHook, isolate);
-
-    // Delete all live contexts
-    for (auto ptr : liveContextPtrs_) {
-      ptr->UnregisterFromGC();
-      delete ptr;
-    }
-    liveContextPtrs_.clear();
-
-    // Delete all unused contexts, too
-    for (auto ptr : deadContextPtrs_) {
-      delete ptr;
-    }
-    deadContextPtrs_.clear();
   }
 }
 
@@ -1354,20 +1296,12 @@ void WallProfiler::SetContext(Isolate* isolate, Local<Value> value) {
     proxyObj->SetPrototype(v8Ctx, proxyProto).Check();
     proxyObj->Set(v8Ctx, cpedProxySymbol_.Get(isolate), cpedObj).Check();
     // Set up the context pointer in the internal field
-    if (!deadContextPtrs_.empty()) {
-      contextPtr = deadContextPtrs_.back();
-      deadContextPtrs_.pop_back();
-    } else {
-      contextPtr = new PersistentContextPtr(this);
-    }
+    contextPtr = new PersistentContextPtr(&liveContextPtrs_, proxyObj);
     liveContextPtrs_.insert(contextPtr);
-    contextPtr->RegisterForGC(isolate, cpedObj);
-    proxyObj->SetAlignedPointerInInternalField(0, contextPtr);
     // Set the proxy object as the continuation preserved embedder data
     isolate->SetContinuationPreservedEmbedderData(proxyObj);
   } else {
-    contextPtr = static_cast<PersistentContextPtr*>(
-        cpedObj->GetAlignedPointerFromInternalField(0));
+    contextPtr = PersistentContextPtr::Unwrap(cpedObj);
   }
   contextPtr->Set(isolate, value);
 #else
@@ -1429,7 +1363,6 @@ ContextPtr WallProfiler::GetContextPtr(Isolate* isolate) {
 
 Local<Object> WallProfiler::GetMetrics(Isolate* isolate) {
   auto usedAsyncContextCount = liveContextPtrs_.size();
-  auto totalAsyncContextCount = usedAsyncContextCount + deadContextPtrs_.size();
   auto context = isolate->GetCurrentContext();
   auto metrics = Object::New(isolate);
   metrics
@@ -1440,7 +1373,7 @@ Local<Object> WallProfiler::GetMetrics(Isolate* isolate) {
   metrics
       ->Set(context,
             String::NewFromUtf8Literal(isolate, "totalAsyncContextCount"),
-            Number::New(isolate, totalAsyncContextCount))
+            Number::New(isolate, usedAsyncContextCount))
       .ToChecked();
   return metrics;
 }
@@ -1550,35 +1483,6 @@ void WallProfiler::OnGCEnd() {
   // Not strictly necessary, as we'll reset it to something else on next GC,
   // but why retain it longer than needed?
   gcContext_.reset();
-
-  const size_t deadCount = deadContextPtrs_.size();
-  deadCountAtPrevGc_ = deadCountAtLastGc_;
-  deadCountAtLastGc_ = deadCount;
-  if (deadCountAtLastGc_ > deadCountAtPrevGc_) {
-    deadStableCycles_ = 0;
-    if (trimBatch_ < kTrimBatchMax) {
-      if (++deadGrowthCycles_ >= 2) {
-        const size_t doubled = trimBatch_ * 2;
-        trimBatch_ = doubled > kTrimBatchMax ? kTrimBatchMax : doubled;
-        deadGrowthCycles_ = 0;
-      }
-    } else {
-      deadGrowthCycles_ = 0;
-    }
-  } else {
-    deadGrowthCycles_ = 0;
-    if (trimBatch_ > kTrimBatchMin) {
-      if (++deadStableCycles_ >= 3) {
-        trimBatch_ = trimBatch_ / 2;
-        if (trimBatch_ < kTrimBatchMin) {
-          trimBatch_ = kTrimBatchMin;
-        }
-        deadStableCycles_ = 0;
-      }
-    } else {
-      deadStableCycles_ = 0;
-    }
-  }
 }
 
 void WallProfiler::PushContext(int64_t time_from,

bindings/profilers/wall.hh

@@ -62,16 +62,6 @@ class WallProfiler : public Nan::ObjectWrap {
   // We track live context pointers in a set to avoid memory leaks. They will
   // be deleted when the profiler is disposed.
   std::unordered_set<PersistentContextPtr*> liveContextPtrs_;
-  // Context pointers belonging to GC'd CPED objects register themselves here.
-  // They will be reused.
-  std::deque<PersistentContextPtr*> deadContextPtrs_;
-  static constexpr size_t kTrimBatchMin = 32;
-  static constexpr size_t kTrimBatchMax = 1024;
-  size_t trimBatch_ = kTrimBatchMin;
-  size_t deadCountAtLastGc_ = 0;
-  size_t deadCountAtPrevGc_ = 0;
-  unsigned int deadGrowthCycles_ = 0;
-  unsigned int deadStableCycles_ = 0;
 
   std::atomic<int> gcCount = 0;
   std::atomic<bool> setInProgress_ = false;
@@ -110,7 +100,7 @@ class WallProfiler : public Nan::ObjectWrap {
   using ContextBuffer = std::vector<SampleContext>;
   ContextBuffer contexts_;
 
-  ~WallProfiler() = default;
+  ~WallProfiler();
   void Dispose(v8::Isolate* isolate, bool removeFromMap);
 
   // A new CPU profiler object will be created each time profiling is started
@@ -189,8 +179,6 @@ class WallProfiler : public Nan::ObjectWrap {
   void OnGCStart(v8::Isolate* isolate);
   void OnGCEnd();
 
-  void MarkDeadPersistentContextPtr(PersistentContextPtr* ptr);
-
   static NAN_METHOD(New);
   static NAN_METHOD(Start);
   static NAN_METHOD(Stop);