perf_hooks: track GC callback installation natively · nodejs/node@c545d04 · GitHub
Skip to content

Commit c545d04

Browse files
jasnelladuh95
jasnell
authored and
aduh95
committed
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>
1 parent 0bd9905 commit c545d04

6 files changed

Lines changed: 121 additions & 39 deletions

File tree

‎lib/internal/perf/observe.js‎

Lines changed: 8 additions & 12 deletions

‎src/node_perf.cc‎

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -239,30 +239,41 @@ void MarkGarbageCollectionEnd(
239239

240240
void GarbageCollectionCleanupHook(void* data) {
241241
Environment* env = static_cast<Environment*>(data);
242+
PerformanceState* state = env->performance_state();
243+
if (!state->gc_tracking_installed) return;
242244
// Reset current_gc_type to 0
243-
env->performance_state()->current_gc_type = 0;
245+
state->current_gc_type = 0;
244246
env->isolate()->RemoveGCPrologueCallback(MarkGarbageCollectionStart, data);
245247
env->isolate()->RemoveGCEpilogueCallback(MarkGarbageCollectionEnd, data);
248+
state->gc_tracking_installed = false;
246249
}
247250

248-
static void InstallGarbageCollectionTracking(
249-
const FunctionCallbackInfo<Value>& args) {
250-
Environment* env = Environment::GetCurrent(args);
251-
// Reset current_gc_type to 0
252-
env->performance_state()->current_gc_type = 0;
253-
env->isolate()->AddGCPrologueCallback(MarkGarbageCollectionStart,
254-
static_cast<void*>(env));
255-
env->isolate()->AddGCEpilogueCallback(MarkGarbageCollectionEnd,
256-
static_cast<void*>(env));
257-
env->AddCleanupHook(GarbageCollectionCleanupHook, env);
251+
// Registers the GC callbacks with V8 if and only if GC timing is needed,
252+
// i.e. there are 'gc' PerformanceObservers. This is idempotent, so it never
253+
// adds the callbacks twice or removes callbacks that are not registered.
254+
static void ReconcileGarbageCollectionTracking(Environment* env) {
255+
PerformanceState* state = env->performance_state();
256+
const bool wanted = state->observers[NODE_PERFORMANCE_ENTRY_TYPE_GC] > 0;
257+
if (wanted == state->gc_tracking_installed) return;
258+
259+
if (wanted) {
260+
// Reset current_gc_type to 0
261+
state->current_gc_type = 0;
262+
env->isolate()->AddGCPrologueCallback(MarkGarbageCollectionStart,
263+
static_cast<void*>(env));
264+
env->isolate()->AddGCEpilogueCallback(MarkGarbageCollectionEnd,
265+
static_cast<void*>(env));
266+
env->AddCleanupHook(GarbageCollectionCleanupHook, env);
267+
state->gc_tracking_installed = true;
268+
} else {
269+
env->RemoveCleanupHook(GarbageCollectionCleanupHook, env);
270+
GarbageCollectionCleanupHook(env);
271+
}
258272
}
259273

260-
static void RemoveGarbageCollectionTracking(
261-
const FunctionCallbackInfo<Value> &args) {
262-
Environment* env = Environment::GetCurrent(args);
263-
264-
env->RemoveCleanupHook(GarbageCollectionCleanupHook, env);
265-
GarbageCollectionCleanupHook(env);
274+
static void UpdateGarbageCollectionTracking(
275+
const FunctionCallbackInfo<Value>& args) {
276+
ReconcileGarbageCollectionTracking(Environment::GetCurrent(args));
266277
}
267278

268279
// Notify a custom PerformanceEntry to observers
@@ -363,12 +374,8 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
363374
SetMethod(isolate, target, "setupObservers", SetupPerformanceObservers);
364375
SetMethod(isolate,
365376
target,
366-
"installGarbageCollectionTracking",
367-
InstallGarbageCollectionTracking);
368-
SetMethod(isolate,
369-
target,
370-
"removeGarbageCollectionTracking",
371-
RemoveGarbageCollectionTracking);
377+
"updateGarbageCollectionTracking",
378+
UpdateGarbageCollectionTracking);
372379
SetMethod(isolate, target, "notify", Notify);
373380
SetMethod(isolate, target, "loopIdleTime", LoopIdleTime);
374381
SetMethod(isolate, target, "createELDHistogram", CreateELDHistogram);
@@ -445,8 +452,7 @@ void CreatePerContextProperties(Local<Object> target,
445452

446453
void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
447454
registry->Register(SetupPerformanceObservers);
448-
registry->Register(InstallGarbageCollectionTracking);
449-
registry->Register(RemoveGarbageCollectionTracking);
455+
registry->Register(UpdateGarbageCollectionTracking);
450456
registry->Register(Notify);
451457
registry->Register(LoopIdleTime);
452458
registry->Register(CreateELDHistogram);

‎src/node_perf_common.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,9 @@ class PerformanceState {
8585

8686
uint64_t performance_last_gc_start_mark = 0;
8787
uint16_t current_gc_type = 0;
88+
// Whether MarkGarbageCollectionStart/End are registered with V8. This is
89+
// not serialized, as V8 GC callbacks do not survive a snapshot.
90+
bool gc_tracking_installed = false;
8891

8992
void Mark(enum PerformanceMilestone milestone,
9093
uint64_t ts = PERFORMANCE_NOW());

‎test/fixtures/snapshot/perf-hooks-gc-observer.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
'use strict';
2+
3+
const { PerformanceObserver } = require('node:perf_hooks');
4+
const { setDeserializeMainFunction } = require('node:v8').startupSnapshot;
5+
6+
// Observe 'gc' entries while building the snapshot.
7+
const observer = new PerformanceObserver(() => {});
8+
observer.observe({ type: 'gc' });
9+
10+
// Performance entries are dispatched asynchronously, so trigger GCs until the
11+
// entries arrive.
12+
function waitForEntries(getCount, callback, attempts = 10) {
13+
globalThis.gc();
14+
setImmediate(() => {
15+
if (getCount() > 0) {
16+
callback();
17+
} else if (attempts > 1) {
18+
waitForEntries(getCount, callback, attempts - 1);
19+
} else {
20+
throw new Error('No gc entries were received after deserialization');
21+
}
22+
});
23+
}
24+
25+
setDeserializeMainFunction(() => {
26+
// The GC callbacks registered while building the snapshot do not survive
27+
// it. Observing 'gc' after deserialization must register them again.
28+
let received = 0;
29+
const newObserver = new PerformanceObserver((list) => {
30+
received += list.getEntries().length;
31+
});
32+
newObserver.observe({ type: 'gc' });
33+
34+
waitForEntries(() => received, () => {
35+
// Disconnecting must only remove GC callbacks that are registered.
36+
newObserver.disconnect();
37+
observer.disconnect();
38+
console.log('ok');
39+
});
40+
});

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
'use strict';
2+
3+
// Tests that 'gc' PerformanceObservers work after deserializing a snapshot
4+
// that was built while a 'gc' PerformanceObserver was active, and that they
5+
// can be disconnected without crashing.
6+
7+
require('../common');
8+
const tmpdir = require('../common/tmpdir');
9+
const fixtures = require('../common/fixtures');
10+
const {
11+
spawnSyncAndAssert,
12+
spawnSyncAndExitWithoutError,
13+
} = require('../common/child_process');
14+
15+
tmpdir.refresh();
16+
const blobPath = tmpdir.resolve('snapshot.blob');
17+
const entry = fixtures.path('snapshot', 'perf-hooks-gc-observer.js');
18+
19+
spawnSyncAndExitWithoutError(process.execPath, [
20+
'--expose-gc',
21+
'--snapshot-blob',
22+
blobPath,
23+
'--build-snapshot',
24+
entry,
25+
], {
26+
cwd: tmpdir.path,
27+
});
28+
29+
spawnSyncAndAssert(process.execPath, [
30+
'--expose-gc',
31+
'--snapshot-blob',
32+
blobPath,
33+
], {
34+
cwd: tmpdir.path,
35+
}, {
36+
stdout: 'ok',
37+
trim: true,
38+
});

‎typings/internalBinding/performance.d.ts‎

Lines changed: 1 addition & 2 deletions

0 commit comments

Comments
 (0)