perf_hooks: fix PerformanceObserver handling

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);

src/node_perf.cc

@@ -228,30 +228,41 @@ void MarkGarbageCollectionEnd(
 
 void GarbageCollectionCleanupHook(void* data) {
   Environment* env = static_cast<Environment*>(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<Value>& 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<void*>(env));
-  env->isolate()->AddGCEpilogueCallback(MarkGarbageCollectionEnd,
-                                        static_cast<void*>(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<void*>(env));
+    env->isolate()->AddGCEpilogueCallback(MarkGarbageCollectionEnd,
+                                          static_cast<void*>(env));
+    env->AddCleanupHook(GarbageCollectionCleanupHook, env);
+    state->gc_tracking_installed = true;
+  } else {
+    env->RemoveCleanupHook(GarbageCollectionCleanupHook, env);
+    GarbageCollectionCleanupHook(env);
+  }
 }
 
-static void RemoveGarbageCollectionTracking(
-  const FunctionCallbackInfo<Value> &args) {
-  Environment* env = Environment::GetCurrent(args);
-
-  env->RemoveCleanupHook(GarbageCollectionCleanupHook, env);
-  GarbageCollectionCleanupHook(env);
+static void UpdateGarbageCollectionTracking(
+    const FunctionCallbackInfo<Value>& 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<Object> 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);

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());

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');
+    });
+  }
+});

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({});
+}

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,
+  });
+}

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(