diff --git a/lib/internal/perf/observe.js b/lib/internal/perf/observe.js index d284d4a54fbc..4a1931dd299e 100644 --- a/lib/internal/perf/observe.js +++ b/lib/internal/perf/observe.js @@ -29,10 +29,9 @@ const { NODE_PERFORMANCE_ENTRY_TYPE_DNS, NODE_PERFORMANCE_ENTRY_TYPE_QUIC, }, - installGarbageCollectionTracking, observerCounts, - removeGarbageCollectionTracking, setupObservers, + updateGarbageCollectionTracking, } = internalBinding('performance'); const { @@ -77,8 +76,6 @@ const kMaybeBuffer = Symbol('kMaybeBuffer'); const kTypeSingle = 0; const kTypeMultiple = 1; -let gcTrackingInstalled = false; - const kSupportedEntryTypes = ObjectFreeze([ 'dns', 'function', @@ -144,24 +141,41 @@ function maybeDecrementObserverCounts(entryTypes) { if (observerType !== undefined) { observerCounts[observerType]--; - if (observerType === NODE_PERFORMANCE_ENTRY_TYPE_GC && - observerCounts[observerType] === 0) { - removeGarbageCollectionTracking(); - gcTrackingInstalled = false; + // Removes the GC callbacks once the last 'gc' observer is gone. + if (observerType === NODE_PERFORMANCE_ENTRY_TYPE_GC) { + updateGarbageCollectionTracking(); } } } } +let gcTrackingDeserializeCallbackAdded = false; + +// V8 GC callbacks do not survive a snapshot. When building one with active +// 'gc' observers, register the callbacks again after deserialization. +function maybeRestoreGarbageCollectionTrackingOnDeserialize() { + if (gcTrackingDeserializeCallbackAdded) return; + const { + namespace: { + addDeserializeCallback, + isBuildingSnapshot, + }, + } = require('internal/v8/startup_snapshot'); + if (!isBuildingSnapshot()) return; + gcTrackingDeserializeCallbackAdded = true; + addDeserializeCallback(updateGarbageCollectionTracking); +} + function maybeIncrementObserverCount(type) { const observerType = getObserverType(type); if (observerType !== undefined) { observerCounts[observerType]++; - if (!gcTrackingInstalled && - observerType === NODE_PERFORMANCE_ENTRY_TYPE_GC) { - installGarbageCollectionTracking(); - gcTrackingInstalled = true; + // Installs the GC callbacks if they are not installed yet. This is + // idempotent, so it is called whenever the 'gc' observer count changes. + if (observerType === NODE_PERFORMANCE_ENTRY_TYPE_GC) { + updateGarbageCollectionTracking(); + maybeRestoreGarbageCollectionTrackingOnDeserialize(); } } } @@ -291,16 +305,23 @@ class PerformanceObserver { maybeDecrementObserverCounts(this.#entryTypes); this.#entryTypes.clear(); for (let n = 0; n < entryTypes.length; n++) { - if (ArrayPrototypeIncludes(kSupportedEntryTypes, entryTypes[n])) { - this.#entryTypes.add(entryTypes[n]); - maybeIncrementObserverCount(entryTypes[n]); + const entryType = entryTypes[n]; + // Count each entry type at most once per observer, as disconnect() + // decrements the counts once per observed type. + if (ArrayPrototypeIncludes(kSupportedEntryTypes, entryType) && + !this.#entryTypes.has(entryType)) { + this.#entryTypes.add(entryType); + maybeIncrementObserverCount(entryType); } } } else { if (!ArrayPrototypeIncludes(kSupportedEntryTypes, type)) return; - this.#entryTypes.add(type); - maybeIncrementObserverCount(type); + // Observing the same type again only replaces the options. + if (!this.#entryTypes.has(type)) { + this.#entryTypes.add(type); + maybeIncrementObserverCount(type); + } if (buffered) { const entries = filterBufferMapByNameAndType(undefined, type); SafeArrayPrototypePushApply(this.#buffer, entries); diff --git a/src/node_perf.cc b/src/node_perf.cc index b4c74e9a09a7..6c2edaf600a7 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -228,30 +228,41 @@ void MarkGarbageCollectionEnd( void GarbageCollectionCleanupHook(void* data) { Environment* env = static_cast(data); + PerformanceState* state = env->performance_state(); + if (!state->gc_tracking_installed) return; // Reset current_gc_type to 0 - env->performance_state()->current_gc_type = 0; + state->current_gc_type = 0; env->isolate()->RemoveGCPrologueCallback(MarkGarbageCollectionStart, data); env->isolate()->RemoveGCEpilogueCallback(MarkGarbageCollectionEnd, data); + state->gc_tracking_installed = false; } -static void InstallGarbageCollectionTracking( - const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - // Reset current_gc_type to 0 - env->performance_state()->current_gc_type = 0; - env->isolate()->AddGCPrologueCallback(MarkGarbageCollectionStart, - static_cast(env)); - env->isolate()->AddGCEpilogueCallback(MarkGarbageCollectionEnd, - static_cast(env)); - env->AddCleanupHook(GarbageCollectionCleanupHook, env); +// Registers the GC callbacks with V8 if and only if GC timing is needed, +// i.e. there are 'gc' PerformanceObservers. This is idempotent, so it never +// adds the callbacks twice or removes callbacks that are not registered. +static void ReconcileGarbageCollectionTracking(Environment* env) { + PerformanceState* state = env->performance_state(); + const bool wanted = state->observers[NODE_PERFORMANCE_ENTRY_TYPE_GC] > 0; + if (wanted == state->gc_tracking_installed) return; + + if (wanted) { + // Reset current_gc_type to 0 + state->current_gc_type = 0; + env->isolate()->AddGCPrologueCallback(MarkGarbageCollectionStart, + static_cast(env)); + env->isolate()->AddGCEpilogueCallback(MarkGarbageCollectionEnd, + static_cast(env)); + env->AddCleanupHook(GarbageCollectionCleanupHook, env); + state->gc_tracking_installed = true; + } else { + env->RemoveCleanupHook(GarbageCollectionCleanupHook, env); + GarbageCollectionCleanupHook(env); + } } -static void RemoveGarbageCollectionTracking( - const FunctionCallbackInfo &args) { - Environment* env = Environment::GetCurrent(args); - - env->RemoveCleanupHook(GarbageCollectionCleanupHook, env); - GarbageCollectionCleanupHook(env); +static void UpdateGarbageCollectionTracking( + const FunctionCallbackInfo& args) { + ReconcileGarbageCollectionTracking(Environment::GetCurrent(args)); } // Notify a custom PerformanceEntry to observers @@ -346,12 +357,8 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "setupObservers", SetupPerformanceObservers); SetMethod(isolate, target, - "installGarbageCollectionTracking", - InstallGarbageCollectionTracking); - SetMethod(isolate, - target, - "removeGarbageCollectionTracking", - RemoveGarbageCollectionTracking); + "updateGarbageCollectionTracking", + UpdateGarbageCollectionTracking); SetMethod(isolate, target, "notify", Notify); SetMethod(isolate, target, "loopIdleTime", LoopIdleTime); SetMethod(isolate, target, "createELDHistogram", CreateELDHistogram); @@ -423,8 +430,7 @@ void CreatePerContextProperties(Local target, void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(SetupPerformanceObservers); - registry->Register(InstallGarbageCollectionTracking); - registry->Register(RemoveGarbageCollectionTracking); + registry->Register(UpdateGarbageCollectionTracking); registry->Register(Notify); registry->Register(LoopIdleTime); registry->Register(CreateELDHistogram); diff --git a/src/node_perf_common.h b/src/node_perf_common.h index aa84ba55b08e..7ad2c9ed7de1 100644 --- a/src/node_perf_common.h +++ b/src/node_perf_common.h @@ -83,6 +83,9 @@ class PerformanceState { uint64_t performance_last_gc_start_mark = 0; uint16_t current_gc_type = 0; + // Whether MarkGarbageCollectionStart/End are registered with V8. This is + // not serialized, as V8 GC callbacks do not survive a snapshot. + bool gc_tracking_installed = false; void Mark(enum PerformanceMilestone milestone, uint64_t ts = PERFORMANCE_NOW()); diff --git a/test/fixtures/snapshot/perf-hooks-gc-observer.js b/test/fixtures/snapshot/perf-hooks-gc-observer.js new file mode 100644 index 000000000000..353f10fc0ee8 --- /dev/null +++ b/test/fixtures/snapshot/perf-hooks-gc-observer.js @@ -0,0 +1,52 @@ +'use strict'; + +const { PerformanceObserver } = require('node:perf_hooks'); +const { setDeserializeMainFunction } = require('node:v8').startupSnapshot; + +// Observe 'gc' entries while building the snapshot. +let received = 0; +const observer = new PerformanceObserver((list) => { + received += list.getEntries().length; +}); +observer.observe({ type: 'gc' }); + +// Performance entries are dispatched asynchronously, so trigger GCs until the +// entries arrive. +function waitForEntries(getCount, callback, attempts = 10) { + globalThis.gc(); + setImmediate(() => { + if (getCount() > 0) { + callback(); + } else if (attempts > 1) { + waitForEntries(getCount, callback, attempts - 1); + } else { + throw new Error('No gc entries were received after deserialization'); + } + }); +} + +setDeserializeMainFunction(() => { + // The GC callbacks registered while building the snapshot do not survive + // it, so they must be registered again after deserialization. + if (process.env.TEST_NEW_OBSERVER) { + // Observing 'gc' again after deserialization. + let newReceived = 0; + const newObserver = new PerformanceObserver((list) => { + newReceived += list.getEntries().length; + }); + newObserver.observe({ type: 'gc' }); + + waitForEntries(() => newReceived, () => { + // Disconnecting must only remove GC callbacks that are registered. + newObserver.disconnect(); + observer.disconnect(); + console.log('ok'); + }); + } else { + // The observer that was active while building the snapshot. + waitForEntries(() => received, () => { + observer.disconnect(); + console.log('ok'); + }); + } +}); diff --git a/test/parallel/test-performanceobserver-observer-counts.js b/test/parallel/test-performanceobserver-observer-counts.js new file mode 100644 index 000000000000..46dc8b0606aa --- /dev/null +++ b/test/parallel/test-performanceobserver-observer-counts.js @@ -0,0 +1,87 @@ +// Flags: --expose-internals +'use strict'; + +// Tests that the observer counts, which gate the creation of performance +// entries, return to zero once observers disconnect, however the entry types +// were observed. + +require('../common'); +const assert = require('node:assert'); +const { PerformanceObserver } = require('node:perf_hooks'); +const { internalBinding } = require('internal/test/binding'); +const { hasObserver } = require('internal/perf/observe'); + +const { + observerCounts, + constants: { + NODE_PERFORMANCE_ENTRY_TYPE_GC, + NODE_PERFORMANCE_ENTRY_TYPE_HTTP, + NODE_PERFORMANCE_ENTRY_TYPE_DNS, + }, +} = internalBinding('performance'); + +const kTypes = { + gc: NODE_PERFORMANCE_ENTRY_TYPE_GC, + http: NODE_PERFORMANCE_ENTRY_TYPE_HTTP, + dns: NODE_PERFORMANCE_ENTRY_TYPE_DNS, +}; + +function assertCounts(expected) { + for (const { 0: type, 1: index } of Object.entries(kTypes)) { + const count = expected[type] ?? 0; + assert.strictEqual(observerCounts[index], count, + `observer count for '${type}'`); + assert.strictEqual(hasObserver(type), count > 0, `hasObserver('${type}')`); + } +} + +assertCounts({}); + +{ + // Observing the same type more than once counts it once. + const obs = new PerformanceObserver(() => {}); + for (const type of ['gc', 'http', 'dns']) { + obs.observe({ type }); + obs.observe({ type }); + } + assertCounts({ gc: 1, http: 1, dns: 1 }); + obs.disconnect(); + assertCounts({}); + // Disconnecting again must not decrement the counts any further. + obs.disconnect(); + assertCounts({}); +} + +{ + // Duplicate entry types are counted once. + const obs = new PerformanceObserver(() => {}); + obs.observe({ entryTypes: ['http', 'http', 'gc', 'gc'] }); + assertCounts({ gc: 1, http: 1 }); + obs.disconnect(); + assertCounts({}); +} + +{ + // Replacing the observed entry types updates the counts. + const obs = new PerformanceObserver(() => {}); + obs.observe({ entryTypes: ['gc', 'http'] }); + assertCounts({ gc: 1, http: 1 }); + obs.observe({ entryTypes: ['http'] }); + assertCounts({ http: 1 }); + obs.disconnect(); + assertCounts({}); +} + +{ + // Each observer is counted separately. + const obs1 = new PerformanceObserver(() => {}); + const obs2 = new PerformanceObserver(() => {}); + obs1.observe({ type: 'gc' }); + obs2.observe({ type: 'gc' }); + obs2.observe({ type: 'gc' }); + assertCounts({ gc: 2 }); + obs1.disconnect(); + assertCounts({ gc: 1 }); + obs2.disconnect(); + assertCounts({}); +} diff --git a/test/parallel/test-snapshot-perf-hooks-gc-observer.js b/test/parallel/test-snapshot-perf-hooks-gc-observer.js new file mode 100644 index 000000000000..ce43592a958c --- /dev/null +++ b/test/parallel/test-snapshot-perf-hooks-gc-observer.js @@ -0,0 +1,44 @@ +'use strict'; + +// Tests that 'gc' PerformanceObservers work after deserializing a snapshot +// that was built while a 'gc' PerformanceObserver was active, and that they +// can be disconnected without crashing. + +require('../common'); +const tmpdir = require('../common/tmpdir'); +const fixtures = require('../common/fixtures'); +const { + spawnSyncAndAssert, + spawnSyncAndExitWithoutError, +} = require('../common/child_process'); + +tmpdir.refresh(); +const blobPath = tmpdir.resolve('snapshot.blob'); +const entry = fixtures.path('snapshot', 'perf-hooks-gc-observer.js'); + +spawnSyncAndExitWithoutError(process.execPath, [ + '--expose-gc', + '--snapshot-blob', + blobPath, + '--build-snapshot', + entry, +], { + cwd: tmpdir.path, +}); + +// The observer that was active while building the snapshot receives entries +// after deserialization. With TEST_NEW_OBSERVER, a new observer is created +// after deserialization instead. +for (const env of [{}, { TEST_NEW_OBSERVER: '1' }]) { + spawnSyncAndAssert(process.execPath, [ + '--expose-gc', + '--snapshot-blob', + blobPath, + ], { + cwd: tmpdir.path, + env: { ...process.env, ...env }, + }, { + stdout: 'ok', + trim: true, + }); +} diff --git a/typings/internalBinding/performance.d.ts b/typings/internalBinding/performance.d.ts index cf3ef0a664f0..83d95b2e66b9 100644 --- a/typings/internalBinding/performance.d.ts +++ b/typings/internalBinding/performance.d.ts @@ -136,8 +136,7 @@ export interface PerformanceBinding { observerCounts: Uint32Array; milestones: Float64Array; setupObservers(callback: PerformanceObserverCallback): void; - installGarbageCollectionTracking(): void; - removeGarbageCollectionTracking(): void; + updateGarbageCollectionTracking(): void; notify(type: string, entry: unknown): void; loopIdleTime(): number; createELDHistogram(