From f1af6c9e55d3781742fdba1cf07e2da7e139e8f1 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Mon, 9 Feb 2026 11:55:05 +0100 Subject: [PATCH 1/2] Do not reuse PersistentContextPtr instances --- bindings/profilers/wall.cc | 148 +++++++------------------------------ bindings/profilers/wall.hh | 14 +--- 2 files changed, 27 insertions(+), 135 deletions(-) diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index e2c24817..d605ff5f 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -105,37 +105,23 @@ void SetContextPtr(ContextPtr& contextPtr, } } -class PersistentContextPtr { +class PersistentContextPtr : public node::ObjectWrap { ContextPtr context; - WallProfiler* owner; - Persistent per; + std::unordered_set* live; public: - PersistentContextPtr(WallProfiler* owner) : owner(owner) {} - - void UnregisterFromGC() { - if (!per.IsEmpty()) { - per.ClearWeak(); - per.Reset(); - } + PersistentContextPtr(std::unordered_set* live, + Local wrap) + : live(live) { + Wrap(wrap); } - void MarkDead() { - context.reset(); - owner->MarkDeadPersistentContextPtr(this); - } + void Detach() { live = nullptr; } - void RegisterForGC(Isolate* isolate, const Local& obj) { - // Register a callback to delete this object when the object is GCed - per.Reset(isolate, obj); - per.SetWeak( - this, - [](const WeakCallbackInfo& data) { - auto ptr = data.GetParameter(); - ptr->UnregisterFromGC(); - ptr->MarkDead(); - }, - WeakCallbackType::kParameter); + ~PersistentContextPtr() { + if (live) { + live->erase(this); + } } void Set(Isolate* isolate, const Local& value) { @@ -143,51 +129,11 @@ class PersistentContextPtr { } 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 wrap) { + return node::ObjectWrap::Unwrap(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) { 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( - cpedObj->GetAlignedPointerFromInternalField(0)); + contextPtr = PersistentContextPtr::Unwrap(cpedObj); } contextPtr->Set(isolate, value); #else @@ -1429,7 +1363,6 @@ ContextPtr WallProfiler::GetContextPtr(Isolate* isolate) { Local 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 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, diff --git a/bindings/profilers/wall.hh b/bindings/profilers/wall.hh index d60bcdf7..0d044d43 100644 --- a/bindings/profilers/wall.hh +++ b/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 liveContextPtrs_; - // Context pointers belonging to GC'd CPED objects register themselves here. - // They will be reused. - std::deque 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 gcCount = 0; std::atomic setInProgress_ = false; @@ -110,7 +100,7 @@ class WallProfiler : public Nan::ObjectWrap { using ContextBuffer = std::vector; 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); From 2e18962b53965d52de1b71bbd8e67fe960fd12f9 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Mon, 9 Feb 2026 13:20:50 +0100 Subject: [PATCH 2/2] No longer necessary test --- ts/test/test-cped-freelist-trimming.ts | 58 -------------------------- 1 file changed, 58 deletions(-) delete mode 100644 ts/test/test-cped-freelist-trimming.ts diff --git a/ts/test/test-cped-freelist-trimming.ts b/ts/test/test-cped-freelist-trimming.ts deleted file mode 100644 index f8863c25..00000000 --- a/ts/test/test-cped-freelist-trimming.ts +++ /dev/null @@ -1,58 +0,0 @@ -/** - * Regression test for CPED context pointer freelist growth. - * - * Runs the actual workload in a separate Node.js process launched with - * `--expose-gc` so we can force GC deterministically. - */ - -import assert from 'assert'; -import {spawnSync} from 'child_process'; -import path from 'path'; -import {satisfies} from 'semver'; - -describe('CPED freelist trimming (regression)', () => { - it('should plateau total async context pointers after enough churn', function () { - this.timeout(120_000); - - if (process.platform !== 'darwin' && process.platform !== 'linux') { - this.skip(); - } - - const supportsCPED = - satisfies(process.versions.node, '>=24.0.0') || - satisfies(process.versions.node, '>=22.7.0'); - if (!supportsCPED) { - this.skip(); - } - - const gcCheck = spawnSync( - process.execPath, - [ - '--expose-gc', - '-e', - "process.exit(typeof global.gc === 'function' ? 0 : 1)", - ], - {stdio: 'pipe'} - ); - if (gcCheck.status !== 0) { - this.skip(); - } - - const child = path.join(__dirname, 'cped-freelist-regression-child.js'); - const args = ['--expose-gc', '--max-old-space-size=4096']; - if ( - satisfies(process.versions.node, '>=22.7.0') && - satisfies(process.versions.node, '<24.0.0') - ) { - args.push('--experimental-async-context-frame'); - } - args.push(child); - const res = spawnSync(process.execPath, args, { - stdio: 'inherit', - }); - - // If the child process exits non-zero, fail with a helpful message. - assert.strictEqual(res.error, undefined); - assert.strictEqual(res.status, 0, `child exited with status ${res.status}`); - }); -});