Skip to content

perf_hooks: fix PerformanceObserver handling - #66097

Open
jasnell wants to merge 3 commits into
nodejs:mainfrom
jasnell:jasnell/perf-hooks-gc-tracking
Open

jasnell wants to merge 3 commits into
nodejs:mainfrom
jasnell:jasnell/perf-hooks-gc-tracking

Conversation

@jasnell

@jasnell jasnell commented Sep 17, 2026

Copy link
Copy Markdown
Member

PerformanceObserver counts could get out of sync. GC callback handling, in particular, could get out of sync when serializing/deserializing a snapshot, leading to a segfault.

Observing the same entry type more than once on a PerformanceObserver,
through repeated `observe({ type })` calls or duplicates in
`entryTypes`, incremented the internal observer count each time, while
`disconnect()` decremented it only once per observed type. The counts
never returned to zero, so GC tracking stayed installed, and entries
for types such as `'http'` and `'dns'` kept being created after all
observers had disconnected.

Count each entry type at most once per observer.

Assisted-by: OpenCode
Signed-off-by: James M Snell <jasnell@gmail.com>
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>
V8 GC callbacks do not survive a snapshot, so a `'gc'`
PerformanceObserver that was active while building a user-land
snapshot received no entries after deserialization, unless another
`'gc'` observer was created.

When `'gc'` is observed while building a snapshot, add a deserialize
callback that registers the GC callbacks again if `'gc'` observers are
still active.

Assisted-by: OpenCode
Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. typings Issues and PRs related to internal TypeScript declarations. labels Sep 17, 2026
@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.72131% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.27%. Comparing base (6a7f94c) to head (81c0ca8).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/perf/observe.js 97.36% 1 Missing ⚠️
src/node_perf.cc 95.65% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66097      +/-   ##
==========================================
- Coverage   90.27%   90.27%   -0.01%     
==========================================
  Files         790      790              
  Lines      271591   271616      +25     
  Branches    51829    51835       +6     
==========================================
+ Hits       245185   245197      +12     
- Misses      16914    16929      +15     
+ Partials     9492     9490       -2     
Files with missing lines Coverage Δ
lib/internal/perf/observe.js 91.55% <97.36%> (+0.13%) ⬆️
src/node_perf.cc 87.15% <95.65%> (-0.20%) ⬇️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 18, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 18, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jasnell jasnell added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. typings Issues and PRs related to internal TypeScript declarations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants