perf_hooks: track GC callback installation natively

Whether the V8 GC callbacks used for `'gc'` performance entries were
installed was tracked by a boolean in JavaScript, separately from the
native state, and the two could get out of sync. After deserializing a
user-land snapshot built while a `'gc'` PerformanceObserver was active,
the boolean claimed the callbacks were installed, although V8 GC
callbacks do not survive a snapshot. Observing `'gc'` again did not
install them, and disconnecting removed callbacks that had never been
registered, crashing the process.

Track the installation state in `PerformanceState` instead, and replace
the install and remove bindings with a single idempotent
`updateGarbageCollectionTracking()` binding, which registers the
callbacks if and only if there are `'gc'` observers.

Assisted-by: OpenCode
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #66097
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Author: jasnell

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,10 +141,9 @@ 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();
       }
     }
   }
@@ -158,10 +154,10 @@ function maybeIncrementObserverCount(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();
     }
   }
 }

src/node_perf.cc

@@ -239,30 +239,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
@@ -363,12 +374,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);
@@ -445,8 +452,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

@@ -85,6 +85,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,40 @@
+'use strict';
+
+const { PerformanceObserver } = require('node:perf_hooks');
+const { setDeserializeMainFunction } = require('node:v8').startupSnapshot;
+
+// Observe 'gc' entries while building the snapshot.
+const observer = new PerformanceObserver(() => {});
+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. Observing 'gc' after deserialization must register them again.
+  let received = 0;
+  const newObserver = new PerformanceObserver((list) => {
+    received += list.getEntries().length;
+  });
+  newObserver.observe({ type: 'gc' });
+
+  waitForEntries(() => received, () => {
+    // Disconnecting must only remove GC callbacks that are registered.
+    newObserver.disconnect();
+    observer.disconnect();
+    console.log('ok');
+  });
+});

test/parallel/test-snapshot-perf-hooks-gc-observer.js

@@ -0,0 +1,38 @@
+'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,
+});
+
+spawnSyncAndAssert(process.execPath, [
+  '--expose-gc',
+  '--snapshot-blob',
+  blobPath,
+], {
+  cwd: tmpdir.path,
+}, {
+  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(