From 867c9bc98e5b94ab719e33d1905552c5fb3a3391 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Wed, 27 May 2026 14:24:57 +0200 Subject: [PATCH 1/3] track live context pointers via an intrusive list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PersistentContextPtr instances were registered in an unordered_set on the WallProfiler so that ~WallProfiler could delete any PCPs that V8 had not GC'd yet — otherwise they show up as leaks under LSAN at process exit, since LSAN's check runs before V8's late-shutdown finalizers. The hash-set was heavier than that single use case deserved: every live PCP cost a back-pointer into the set (8 bytes) plus a separate hash node + bucket overhead (~40 bytes) external to the object. Replace it with an intrusive doubly-linked list threaded through the PCPs themselves. Each PCP carries: WallProfiler* const profiler_ // 8 bytes PersistentContextPtr** pprev_ // 8 bytes PersistentContextPtr* next_ // 8 bytes ~PCP unlinks itself when pprev_ != nullptr — the "still on a live profiler's list" predicate. ~WallProfiler walks the list and nulls each pprev_ before delete, so the destructor that fires as part of the walk becomes a no-op rather than poking at the dying profiler's head pointer. Outside of teardown, pprev_ != nullptr is also the proof that the back-pointer is still valid, so ~PCP can dereference profiler_ to decrement its counter without any thread-local or ID lookup. PCP-tracking memory drops from ~48 bytes/PCP (8-byte back-pointer into set + ~40 bytes of hash node / bucket) to 24 bytes/PCP (back- pointer + two list pointers), a saving of ~24 bytes per live async context. Replace GetMetrics's liveContextPtrs_.size() read with an atomic counter incremented/decremented alongside the list link/unlink so the metric path doesn't depend on the container. usedAsyncContextCount / totalAsyncContextCount keep their existing semantics (both report the live count, as before). --- bindings/profilers/wall.cc | 81 +++++++++++++++++++++++++++----------- bindings/profilers/wall.hh | 32 +++++++++++++-- 2 files changed, 88 insertions(+), 25 deletions(-) diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index b6e836c9..8d4202c6 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -108,22 +108,25 @@ void SetContextPtr(ContextPtr& contextPtr, class PersistentContextPtr : public node::ObjectWrap { ContextPtr context; - std::unordered_set* live; + // Back-pointer to the WallProfiler that created this PCP. Guaranteed to be + // a valid pointer whenever pprev_ != nullptr — ~WallProfiler nulls pprev_ + // on every live PCP before any of them can outlive the profiler. + WallProfiler* const profiler_; + // Intrusive doubly-linked list, threaded through the WallProfiler's + // liveContextPtrHead_. pprev_ points at the slot that currently holds *this + // (either the head field or another PCP's next_), enabling O(1) unlink in + // ~PCP. pprev_ == nullptr is the "detached" sentinel — set by + // ~WallProfiler before deleting us, so our unlink becomes a no-op and we + // don't poke at the dying profiler's memory. + PersistentContextPtr** pprev_ = nullptr; + PersistentContextPtr* next_ = nullptr; + + friend class WallProfiler; public: - PersistentContextPtr(std::unordered_set* live, - Local wrap) - : live(live) { - Wrap(wrap); - } + PersistentContextPtr(WallProfiler* profiler, Local wrap); - void Detach() { live = nullptr; } - - ~PersistentContextPtr() { - if (live) { - live->erase(this); - } - } + ~PersistentContextPtr(); void Set(Isolate* isolate, const Local& value) { SetContextPtr(context, isolate, value); @@ -136,6 +139,32 @@ class PersistentContextPtr : public node::ObjectWrap { } }; +PersistentContextPtr::PersistentContextPtr(WallProfiler* profiler, + Local wrap) + : profiler_(profiler) { + Wrap(wrap); + // Splice ourselves at the head of profiler's live list. + auto** headSlot = profiler->liveContextPtrHeadSlot(); + next_ = *headSlot; + pprev_ = headSlot; + if (next_ != nullptr) next_->pprev_ = &next_; + *headSlot = this; + profiler->recordContextCreate(); +} + +PersistentContextPtr::~PersistentContextPtr() { + // pprev_ != nullptr means we're still on profiler_'s live list, which means + // ~WallProfiler hasn't run yet and the back-pointer is still valid. Unlink + // and release the slot in one step. If pprev_ is null, ~WallProfiler is + // taking care of us so don't do anything (don't unlink us, and leave its + // counter alone as it's about to go away). + if (pprev_ != nullptr) { + *pprev_ = next_; + if (next_ != nullptr) next_->pprev_ = pprev_; + profiler_->recordContextRelease(); + } +} + inline void* GetAlignedPointerFromInternalField(Object* object, int index) { #if NODE_MAJOR_VERSION >= 26 return object->GetAlignedPointerFromInternalField( @@ -617,12 +646,21 @@ WallProfiler::~WallProfiler() { } g_profilers.Remove(this); - // Delete all live contexts - for (auto ptr : liveContextPtrs_) { - ptr->Detach(); // so it doesn't invalidate our iterator - delete ptr; - } - liveContextPtrs_.clear(); + // Delete every PCP still live in the CPED map. ~PCP would normally unlink + // itself via pprev_/next_, but we're tearing down the list we point into — + // so null pprev_ first to signal "already detached" and let ~PCP skip the + // unlink. (~ObjectWrap will still clear V8's weak callback during delete, + // so the dangling internal-field pointer in the wrap object stays inert + // even if V8 later GCs the wrap.) + auto* p = liveContextPtrHead_; + while (p != nullptr) { + auto* next = p->next_; + p->pprev_ = nullptr; + p->next_ = nullptr; + delete p; + p = next; + } + liveContextPtrHead_ = nullptr; } void WallProfiler::Dispose(Isolate* isolate) { @@ -1207,8 +1245,7 @@ Local WallProfiler::CreateContextHolder(Isolate* isolate, // for easy access from JS when cpedKey is an ALS, it can do // als.getStore()?.[0]; wrap->Set(v8Ctx, 0, value).Check(); - auto contextPtr = new PersistentContextPtr(&liveContextPtrs_, wrap); - liveContextPtrs_.insert(contextPtr); + auto contextPtr = new PersistentContextPtr(this, wrap); contextPtr->Set(isolate, value); return wrap; } @@ -1273,7 +1310,7 @@ ContextPtr WallProfiler::GetContextPtr(Isolate* isolate) { } Local WallProfiler::GetMetrics(Isolate* isolate) { - auto usedAsyncContextCount = liveContextPtrs_.size(); + auto usedAsyncContextCount = liveContextPtrCount(); auto context = isolate->GetCurrentContext(); auto metrics = Object::New(isolate); metrics diff --git a/bindings/profilers/wall.hh b/bindings/profilers/wall.hh index f40ccdbd..d55d0b1d 100644 --- a/bindings/profilers/wall.hh +++ b/bindings/profilers/wall.hh @@ -57,9 +57,19 @@ class WallProfiler : public Nan::ObjectWrap { int cpedKeyHash_ = 0; v8::Global wrapObjectTemplate_; - // We track live context pointers in a set to avoid memory leaks. They will - // be deleted when the profiler is disposed. - std::unordered_set liveContextPtrs_; + // The list lets ~WallProfiler delete any PCPs V8 hasn't GC'd yet, so they + // don't show up as leaks under LSAN at exit. + // PCPs link themselves in at construction and unlink in their destructor. + // This used to be an unordered set, but storing two pointers in the PCP is + // much cheaper memorywise than the hash set's overhead of buckets and nodes. + PersistentContextPtr* liveContextPtrHead_ = nullptr; + + // Number of PersistentContextPtr instances created by this profiler that + // are still alive. Incremented in PersistentContextPtr's constructor; + // decremented in its destructor when it unlinks from the list (i.e., while + // the profiler is still alive). PCPs cleaned up by ~WallProfiler's walk + // skip the decrement — the counter is going away with the profiler anyway. + std::atomic liveContextPtrCount_{0}; std::atomic gcCount = 0; std::atomic setInProgress_ = false; @@ -175,6 +185,22 @@ class WallProfiler : public Nan::ObjectWrap { bool interceptSignal() const { return withContexts_ || workaroundV8Bug_; } + // Returns the address of the live-PCP list head so a newly constructed PCP + // can splice itself in. Only used by PersistentContextPtr's constructor. + PersistentContextPtr** liveContextPtrHeadSlot() { + return &liveContextPtrHead_; + } + + void recordContextCreate() { + liveContextPtrCount_.fetch_add(1, std::memory_order_relaxed); + } + void recordContextRelease() { + liveContextPtrCount_.fetch_sub(1, std::memory_order_relaxed); + } + size_t liveContextPtrCount() const { + return liveContextPtrCount_.load(std::memory_order_relaxed); + } + int v8ProfilerStuckEventLoopDetected() const { return v8ProfilerStuckEventLoopDetected_; } From 12c673e9b46b18815f18cb904db64e74fe935df2 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Mon, 1 Jun 2026 10:14:52 +0200 Subject: [PATCH 2/3] drop atomic from liveContextPtrCount_ Every access happens on the isolate's thread: incremented by the PCP constructor (called from JS land via SetContext), decremented by the PCP destructor (V8 GC weak callbacks run on the isolate's thread), and read by GetMetrics (also JS-invoked). The signal handler doesn't touch this field. A plain size_t describes the threading model accurately, and the atomic was overkill. --- bindings/profilers/wall.hh | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/bindings/profilers/wall.hh b/bindings/profilers/wall.hh index d55d0b1d..2196c521 100644 --- a/bindings/profilers/wall.hh +++ b/bindings/profilers/wall.hh @@ -69,7 +69,11 @@ class WallProfiler : public Nan::ObjectWrap { // decremented in its destructor when it unlinks from the list (i.e., while // the profiler is still alive). PCPs cleaned up by ~WallProfiler's walk // skip the decrement — the counter is going away with the profiler anyway. - std::atomic liveContextPtrCount_{0}; + // + // Not atomic: all accesses happen on the isolate's thread (construction via + // SetContext from JS, destruction via V8 weak callbacks on the same thread, + // metric reads from JS). Signal handler does not touch this field. + size_t liveContextPtrCount_ = 0; std::atomic gcCount = 0; std::atomic setInProgress_ = false; @@ -191,15 +195,9 @@ class WallProfiler : public Nan::ObjectWrap { return &liveContextPtrHead_; } - void recordContextCreate() { - liveContextPtrCount_.fetch_add(1, std::memory_order_relaxed); - } - void recordContextRelease() { - liveContextPtrCount_.fetch_sub(1, std::memory_order_relaxed); - } - size_t liveContextPtrCount() const { - return liveContextPtrCount_.load(std::memory_order_relaxed); - } + void recordContextCreate() { ++liveContextPtrCount_; } + void recordContextRelease() { --liveContextPtrCount_; } + size_t liveContextPtrCount() const { return liveContextPtrCount_; } int v8ProfilerStuckEventLoopDetected() const { return v8ProfilerStuckEventLoopDetected_; From 7712104e6da4653002e3edb0b7eee95e72274f70 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Mon, 1 Jun 2026 10:28:35 +0200 Subject: [PATCH 3/3] clarify pprev_ comment Old wording said pprev_ "points at the slot that currently holds *this", which is misleading on two fronts: the slot holds the pointer to this PCP, not the object itself, and the comment didn't say why we use a pointer-to-pointer rather than a plain back-pointer to the predecessor. Spell out the invariant (*pprev_ == this) and the reason (one unlink shape for head and interior nodes). --- bindings/profilers/wall.cc | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index 8d4202c6..d7d0d106 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -113,11 +113,15 @@ class PersistentContextPtr : public node::ObjectWrap { // on every live PCP before any of them can outlive the profiler. WallProfiler* const profiler_; // Intrusive doubly-linked list, threaded through the WallProfiler's - // liveContextPtrHead_. pprev_ points at the slot that currently holds *this - // (either the head field or another PCP's next_), enabling O(1) unlink in - // ~PCP. pprev_ == nullptr is the "detached" sentinel — set by - // ~WallProfiler before deleting us, so our unlink becomes a no-op and we - // don't poke at the dying profiler's memory. + // liveContextPtrHead_. pprev_ is the address of the pointer that currently + // references this PCP — &profiler_->liveContextPtrHead_ if we're the head, + // &predecessor->next_ otherwise — so by construction *pprev_ == this. + // Storing the address-of (rather than the predecessor itself) lets ~PCP + // unlink without a head/non-head branch: *pprev_ = next_ followed by + // next_->pprev_ = pprev_ does both ends in one shape. pprev_ == nullptr + // is the "detached" sentinel — set by ~WallProfiler before deleting us, + // so our unlink becomes a no-op and we don't poke at the dying profiler's + // memory. PersistentContextPtr** pprev_ = nullptr; PersistentContextPtr* next_ = nullptr;