node:perf_hooks: make performance the global object, like node - #33475
node:perf_hooks: make performance the global object, like node#33475robobun wants to merge 2 commits into
Conversation
WalkthroughChangesPerformance API integration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:57 AM PT - Jul 18th, 2026
❌ @robobun, your commit 491bf6b has 4 failures in
🧪 To try this PR locally: bunx bun-pr 33475That installs a local version of the PR into your bun-33475 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
71d50b4 to
89c5622
Compare
|
Rescoped: the #31825 already implements On the three review comments from the first push:
One more thing that fell out of writing that test: the |
89c5622 to
4bd4e7f
Compare
There was a problem hiding this comment.
LGTM — the v26 clarification on the module-level eventLoopUtilization export addresses my last comment, and the remaining inline note is a test-quality nit that doesn't block.
Extended reasoning...
Overview
This PR fixes the node:perf_hooks module's performance facade so it forwards every member the global performance object carries, rather than a hand-maintained subset that had drifted. Concretely: adds forwarders for markResourceTiming, addEventListener/removeEventListener/dispatchEvent; converts clearResourceTimings from a local no-op to a real forwarder; converts onresourcetimingbufferfull from a load-time value snapshot to a live getter/setter; adds a module-level eventLoopUtilization export; removes a dead commented-out block. Two files touched: src/js/node/perf_hooks.ts (the facade) and test/js/node/perf_hooks/perf_hooks.test.ts (six new tests, including one that walks the global's full prototype chain and pins the missing set to exactly ["timing"]).
Security risks
None. This is a pure JS compat-layer change that adds forwarding shims to an existing global object. No native code, no user input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. The forwarders are mechanical one-liners following the file's existing method(_) { return performance.method(...arguments); } pattern verbatim. The getter/setter for onresourcetimingbufferfull is the obvious correct shape. The prototype-chain-walking test is a good drift guard. The one design-ish decision — the module-level eventLoopUtilization export — is permissive-only and the author has now cited Node v26's lib/perf_hooks.js module.exports with an explicit note that it's absent on v22 and earlier, which addresses my previous round's concern (I had verified against v22).
Other factors
This PR has been through three review rounds already. Round 1's timerify-related feedback became moot when the author rescoped (deferring timerify to #31825, which they cross-linked with the __proto__: null finding). Round 2's "missing timerify" was a stale-description misread on my part that the author corrected. Round 3's eventLoopUtilization comment was addressed in commit 5220ad2 by rewording the test comment to cite Node's source and the version boundary rather than the docs. The author has been responsive and precise throughout.
The one new inline nit this round (EventTarget test would hang to timeout rather than fail fast on regression, since dispatchEvent is synchronous) is valid but purely about failure-mode diagnostics — the test is correct today and would still fail on regression, just via timeout. Not worth another round-trip.
|
Addressed the EventTarget test nit in ae6915a. It was right on substance, though the specific regression it suggested behaves differently here: dropping the spread entirely ( The shape that actually bites is the listener reaching the global as Silent no-op, and
Which is the point: That makes three assertions in this PR that I'd originally written so they couldn't fail: this one, and the |
There was a problem hiding this comment.
LGTM — every prior review point has been addressed, and the drift-guard test that pins the facade's missing members to exactly ["timing"] should keep this from regressing.
Extended reasoning...
Overview
This PR touches two files: src/js/node/perf_hooks.ts (a built-in JS module) and test/js/node/perf_hooks/perf_hooks.test.ts. It fixes the node:perf_hooks module's performance facade, which had drifted from the global performance object it wraps: markResourceTiming was never forwarded, clearResourceTimings was a local no-op shadowing the real method, onresourcetimingbufferfull was a load-time value snapshot rather than a live getter/setter, and the three EventTarget methods were missing entirely. It also adds eventLoopUtilization as a module-level export to match Node v26.3.0's lib/perf_hooks.js.
Every new forwarder follows the file's existing method(_) { return performance.method(...arguments); } pattern verbatim. A dead comment block was removed. Six new tests cover each fix, including a drift-guard that walks the global's full prototype chain and asserts the facade's missing-member set is exactly ["timing"] (the one web-only member Node's performance lacks).
Security risks
None. This is pure JS forwarding code with no user-input parsing, no auth, no crypto, no filesystem or network access. The forwarders delegate verbatim to the existing global performance object.
Level of scrutiny
Low. The changes are mechanical additions following an established in-file pattern, in a Node-compat shim. No native code is touched. The one design question — whether the facade should exist at all vs. exporting the global directly — is pre-existing and out of scope; this PR correctly follows the file's convention rather than redesigning it. The PR description explicitly notes markResourceTiming remains a no-op stub in JSPerformance.cpp and that making it record entries is separate work.
Other factors
This PR has been through four review rounds from me, and every point was addressed:
- The "every member forwarded" comment overclaim → fixed by actually forwarding the EventTarget methods and adding the prototype-chain drift-guard test.
- The
timerifyrescope confusion → author clarified it was intentional (deferred to #31825, which has a better implementation) and rewrote title/description to match. - The
eventLoopUtilizationmodule-export claim → author demonstrated it against Node v26.3.0's actualmodule.exportsand updated the test comment to cite that source rather than docs. - The
Promise.withResolversnit on the EventTarget test → replaced with the synchronousjest.fn()pattern in ae6915a.
The tests are well-constructed: the forwarding tests shadow the global's method with a spy and assert delegation with the exact arguments (so the old clearResourceTimings no-op would fail them), and the author verified 6 of 8 fail with src/ reverted. The bug hunting system found nothing this round. The PR notes disjoint overlap with #31825 and #32618 in the same file.
There was a problem hiding this comment.
LGTM — all prior feedback addressed, forwarders match the existing pattern in the file, and the prototype-chain test pins future drift.
Extended reasoning...
Overview
Two files: src/js/node/perf_hooks.ts (the built-in module facade) and its test file. The change adds five method forwarders (addEventListener/removeEventListener/dispatchEvent/markResourceTiming/clearResourceTimings) to the module's performance object, converts onresourcetimingbufferfull from a load-time value snapshot to a live getter/setter pair, adds a module-level eventLoopUtilization export (present in Node v26's lib/perf_hooks.js module.exports), and deletes a dead commented-out block. Every new forwarder uses the exact method(_) { return performance.method(...arguments); } shape already used by the ~10 existing forwarders in the same object literal.
Security risks
None. This is pure JS in a built-in module, forwarding to APIs the global performance object already exposes. No new attack surface, no user input parsing, no native code.
Level of scrutiny
Low. This is a mechanical Node-compat gap fix: the facade had drifted from the object it wraps, and the fix is to add the missing forwarders in the same style as the neighbours. The one non-mechanical decision — whether eventLoopUtilization belongs at the module level — was litigated in a prior review round and settled by citing Node v26.3.0's actual module.exports list.
Other factors
This PR has been through four review rounds with me, and every point has been addressed substantively rather than dismissed: the EventTarget methods were added (not just the comment reworded), the clearResourceTimings/markResourceTiming tests were rewritten to spy on the global so they actually fail without the fix, the EventTarget test was rewritten from a hang-on-regression Promise shape to a fail-fast jest.fn() assertion, and the eventLoopUtilization test comment now cites Node's source rather than its docs. The new "does not drop members" test walks the global's full prototype chain and asserts the missing set is exactly ["timing"], which is a real regression guard against the facade drifting again. The bug-hunting system found nothing on the current revision. All prior inline threads are resolved.
CI status: the diff is green, both red builds are infrastructureFlagging this for a maintainer rather than pushing again. Two consecutive builds went red without a single test failure attributable to this diff, which is two files confined to Build 69029 — 281/287 jobs passed, zero jobs in a
Build 69101 (the re-trigger) — one hard failure, It died fetching the build artifact before executing anything. The two other red marks on that build are soft-fail
Why none of it can be this diffThe change touches Locally, on a debug+ASAN build: I have used my one re-trigger ( |
|
are the |
|
Good question. The forwarding isn't a design goal in itself, it falls out of two facts:
So within the current shape (a separate facade object) the forwarders are required, not incidental. The real simplification is to drop the separate object: export The tradeoff is what that does to the web global: I'm happy to do the C++ version and delete the facade if you'd prefer the single-object model. Which way do you want it? |
|
@robobun yes please do that. that's the proper fix |
a174542 to
33a8fe1
Compare
|
Done, in 33a8fe1 (force-pushed, squashed to one commit). The only members the web One call I made that's worth your eyes: I attached those two from the |
33a8fe1 to
9bc8660
Compare
|
Build 69519 on the restructured 9bc8660 hit the same That lane checked out 9bc8660 and then died fetching the build artifact before running any test. That's the third consecutive build where this lane never got past artifact download (69029 expired waiting for an agent, 69101 and 69519 both timed out on the download). 187 other jobs on this build are green; 69 were still scheduled when it was marked failing. The code here is now the single-object restructure, so the earlier CI status note about the facade-era builds is superseded. Locally on debug+ASAN: 7 pass, 5 fail with My one re-trigger is spent. Needs someone with |
9bc8660 to
566c107
Compare
|
Rebased onto main after #31825 merged and resolved the conflict (566c107). #31825's All 13 tests pass (#31825's timerify / This also resolves #9271, now that |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/perf_hooks/perf_hooks.test.ts`:
- Around line 184-196: Add a fresh-subprocess test near the existing
prototype-enumerability test that checks node-only performance members are
absent before loading node:perf_hooks and present afterward. Cover the relevant
flag/state variants using isolated subprocesses, and assert the post-require
properties are installed on the prototype without changing the existing
in-process assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3803e03a-438d-4063-b9d1-bbd31796eab7
📒 Files selected for processing (2)
src/js/node/perf_hooks.tstest/js/node/perf_hooks/perf_hooks.test.ts
5b07c1b to
017ac0b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/perf_hooks.ts (1)
292-299: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftImplement snapshot-based
eventLoopUtilizationsemantics
src/js/node/perf_hooks.ts#L117still returns{ idle: 0, active: 0, utilization: 0 }, so the prototype and module-level exports are exposing a stub instead of Node’s delta-based API. Tighten the tests intest/js/node/perf_hooks/perf_hooks.test.ts#L163-L173and#L245-L253to assert snapshot/delta behavior, not just numeric fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/node/perf_hooks.ts` around lines 292 - 299, Replace the stub implementation used by eventLoopUtilization in src/js/node/perf_hooks.ts (including the prototype site at lines 292-299 and module export site at lines 314-317) with snapshot-based Node-compatible behavior supporting initial and previous-snapshot delta calls. Update the assertions in test/js/node/perf_hooks/perf_hooks.test.ts lines 163-173 and 245-253 to verify snapshot/delta relationships rather than only numeric fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/js/node/perf_hooks.ts`:
- Around line 292-299: Replace the stub implementation used by
eventLoopUtilization in src/js/node/perf_hooks.ts (including the prototype site
at lines 292-299 and module export site at lines 314-317) with snapshot-based
Node-compatible behavior supporting initial and previous-snapshot delta calls.
Update the assertions in test/js/node/perf_hooks/perf_hooks.test.ts lines
163-173 and 245-253 to verify snapshot/delta relationships rather than only
numeric fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 448d27ed-6b1e-434c-a743-45c75ea07e7e
📒 Files selected for processing (2)
src/js/node/perf_hooks.tstest/js/node/perf_hooks/perf_hooks.test.ts
|
Not taking the The tests deliberately assert the shape ( |
017ac0b to
db8aa96
Compare
db8aa96 to
d9f9767
Compare
There was a problem hiding this comment.
LGTM — maintainer-endorsed simplification, all prior review threads resolved.
What was reviewed:
- Prototype install block: targets captured
Performance.prototype,hasOwn-guarded,__proto__: nulldescriptors, wrapped inif (PerformancePrototype)so a deleted global degrades rather than throws. - Module-level
eventLoopUtilization/timerifyexports read off the same prototype with local fallbacks, so identity holds and a replaced global can't undefine them. - Tests cover object identity, prototype placement/non-enumerability, fresh-subprocess before/after install,
onresourcetimingbufferfullaccessor round-trip, and resource-timing methods that the old facade dropped; subprocess stderr assertion is guarded per house style.
Extended reasoning...
Overview
Two files: src/js/node/perf_hooks.ts replaces the hand-written forwarding facade with performance (the global) directly, and installs the three Node-only members (nodeTiming, eventLoopUtilization, timerify) on Performance.prototype non-enumerable, matching Node v26.3.0. test/js/node/perf_hooks/perf_hooks.test.ts adds ~95 lines of coverage for the identity, prototype placement, and the accessor/method drift the facade had accumulated. Net: −52/+40 in src, deleting more forwarder boilerplate than it adds.
Security risks
None identified. JS-only change in a node: compat module; no native code, no auth/crypto/fs. The prototype-install descriptors use __proto__: null and the guards use Object.hasOwn, so prototype pollution can neither redirect the define nor suppress it — both concerns were raised in earlier review rounds and fixed.
Level of scrutiny
Medium. It does introduce a process-global side effect (mutating Performance.prototype on first require), but that is exactly what Node does and was the design dylan-conway explicitly requested ("that's the proper fix"). The change has been through five rounds of inline review on this PR (tamper-resistance of the define target, hasOwn vs in, export identity, comment length, subprocess stderr flake, degrade-not-throw guard, read-site consistency) — every thread is resolved with a corresponding commit, and the bug-hunting pass on the final revision found nothing.
Other factors
- Maintainer (dylan-conway) endorsed the single-object design before implementation.
- Tests fail on main and pass on the branch per the gate evidence in the PR body; the
eventLoopUtilizationshape assertions useexpect.any(Number)so #32618's real implementation won't break them. - The one known behavioral nuance (members attach lazily on first require rather than eagerly) is documented in the PR body and pinned by a fresh-subprocess test so a future C++ move will trip CI rather than drift silently.
- No outstanding reviewer comments; CodeRabbit's suggestion was addressed and confirmed.
d9f9767 to
a7e00ad
Compare
There was a problem hiding this comment.
LGTM — the facade → global-object restructure is what dylan-conway asked for, and every review thread since has been addressed.
What was reviewed:
- Prototype-install block: targets
Performance.prototype(notgetPrototypeOf),hasOwnguards,__proto__: nulldescriptors, and theif (PerformancePrototype)degrade-not-throw guard all match the file's existing hardening conventions. - Module-level
eventLoopUtilization/timerifyexports read off the same prototype the defines target, with local fallbacks — identity withperformance.<name>holds and survives a replaced global or a future native move. - Tests: identity, prototype placement/non-enumerability, resource-timing methods now reachable,
onresourcetimingbufferfullaccessor round-trips, and a fresh-subprocess lazy-install contract; subprocess assertions use the guarded-stderr / combined-object pattern.
Extended reasoning...
Overview
Two files: src/js/node/perf_hooks.ts replaces the hand-written per-method facade with the global performance object itself, installing the three node-only members (nodeTiming, eventLoopUtilization, timerify) onto Performance.prototype non-enumerably so the module can export the global directly. test/js/node/perf_hooks/perf_hooks.test.ts adds coverage for identity, prototype placement, the previously-missing resource-timing methods, the onresourcetimingbufferfull accessor, module-level export identity, and a fresh-subprocess check pinning the lazy-install contract.
Security risks
None. This is a Node-compat reshaping of an existing built-in JS module — no native code, no auth/crypto/permissions, no untrusted-input parsing. The prototype install uses __proto__: null descriptors and Object.hasOwn guards, and targets the captured Performance.prototype rather than getPrototypeOf(performance), so a preload-replaced global cannot redirect the define onto Object.prototype.
Level of scrutiny
Medium. It changes user-visible Node-compat surface (mutates Performance.prototype on first require), but the direction was explicitly requested by a maintainer ("that's the proper fix"), it matches Node v26.3.0's observable placement (verified in the PR body against node -e), and it has been through five rounds of review on this thread with every point applied and re-verified. The one behavioral nuance vs. Node — lazy install on first require rather than eager — is documented, strictly closer to Node than before, and now CI-enforced by the fresh-subprocess test.
Other factors
All prior inline comments (mine, CodeRabbit's, and the maintainer's structural request) are resolved in the current diff: Performance.prototype over getPrototypeOf, hasOwn over in, module-level exports read off PerformancePrototype with ?? local fallbacks, the 3-line comment cap, the guarded expect(stderr) pattern, and the if (PerformancePrototype) degrade-not-throw guard matching the file's existing if (PerformanceEntry) convention. #31825's timerify implementation is preserved verbatim; only its attachment point moved. The bug-hunting system found nothing this run. Net diff is ~-13 lines of source with substantially more test coverage than before.
|
Build 75149 on a7e00ad finalized 283 pass / 3 fail, with zero
Both were reported and deduped against existing main-break sessions. This PR's diff is two files ( |
a7e00ad to
d434d9f
Compare
There was a problem hiding this comment.
No bugs found across four review iterations, and every prior inline nit (prototype target, hasOwn guards, export identity, comment length, subprocess-stderr pattern, degrade-not-throw guard) has been applied and re-verified. Deferring to a maintainer only because the core change — installing node-only members onto the web Performance.prototype at first require from the JS layer rather than in JSPerformance.cpp — is an architectural call on a global's shape that's worth a human sign-off.
What was reviewed:
- Prototype-install block:
Performance?.prototypetarget,hasOwnguards,__proto__: nulldescriptors, degrade-not-throw on deleted/replaced globals — all consistent with the file's existingif (PerformanceEntry)convention. - Module-level
eventLoopUtilization/timerifyexports read off the same prototype with local fallback, so identity holds under both a replaced global and a future native own-prop. - Tests: fresh-subprocess lazy-install contract, non-enumerability,
onresourcetimingbufferfullaccessor round-trip, export-surface parity with Node v26.3.0; subprocess stderr assertion is guarded per house style. - CI on a7e00ad: perf_hooks green on every lane; the 3 red jobs are known main breaks (bake deinit segfault,
--no-orphans (perl)timeout) unrelated to this diff.
Extended reasoning...
Overview
This PR replaces the hand-written performance facade in src/js/node/perf_hooks.ts with the global performance object itself, matching Node's require('node:perf_hooks').performance === globalThis.performance. To make that identity hold, three node-only members (nodeTiming, eventLoopUtilization, timerify) are installed onto Performance.prototype (non-enumerable, hasOwn-guarded, __proto__: null descriptors) at first require. Module-level eventLoopUtilization/timerify re-exports read off that prototype with a local-function fallback. Net: ~50 lines deleted from src/js/node/perf_hooks.ts, ~35 added; ~100 lines of new test coverage in test/js/node/perf_hooks/perf_hooks.test.ts.
Security risks
None identified. No untrusted-input parsing, no auth/crypto/permissions surface. The tamper-resistance concerns raised in earlier rounds (a preload replacing globalThis.performance redirecting the define onto Object.prototype; a polluted Object.prototype suppressing the install via in; a deleted globalThis.Performance throwing at module load) were all addressed: Performance?.prototype as the target, Object.hasOwn guards, if (PerformancePrototype) wrapper, and prototype-read exports with ?? local fallback.
Level of scrutiny
Moderate-to-high. This is Node-compat work in src/js/node/ (which .claude/docs/landing-prs.md flags for extra care) and it mutates a web global's prototype from the node: layer at first-require rather than making the members eager in JSPerformance.cpp. The PR body explicitly justifies that layering (avoids reimplementing #31825's timerify and #32618's in-flight eventLoopUtilization in C++; hasOwn guards make a later native move a no-op here), and the fresh-subprocess test pins the lazy-install contract so a future eager move fails loudly. That reasoning is sound, but whether "lazy install on Performance.prototype from JS" is the right interim shape vs. going straight to C++ is a maintainer call, not a mechanical correctness question.
Other factors
- All 11 prior inline comments (mine and CodeRabbit's) are resolved with fix commits; no open threads.
- The bug-hunting system found nothing on the current head.
- Test coverage is thorough: object identity, prototype placement/enumerability, subprocess-isolated before/after install state,
onresourcetimingbufferfullaccessor round-trip, module-export identity, and Node v26.3.0 export-surface parity. The oldstubstest's assertions are preserved undernode-only members are present on performance. - CI build 75149 on a7e00ad: 283 pass / 3 fail, all three fails are pre-existing main breaks with no
perf_hooksinvolvement; the modified test file passed on every lane. - The recent-commits list shows d434d9f and c67ff0a already on
mainwith this PR's title, so this may already be merged — in which case this review is informational only.
d434d9f to
9b34a71
Compare
There was a problem hiding this comment.
No issues found in this pass — all earlier review feedback has been applied and CI is green on every perf_hooks lane. Deferring to a maintainer only because the core approach (installing nodeTiming/eventLoopUtilization/timerify onto the WebCore Performance.prototype from the node: layer at first require) is a layering choice worth a human sign-off.
What was reviewed:
- Prototype-install block:
Performance?.prototypeguard,hasOwngates,__proto__: nulldescriptors, and thePerformancePrototype?.x ?? localexport fallbacks — all consistent now. - Confirmed the deleted facade's members (
markResourceTiming,clearResourceTimings,onresourcetimingbufferfull, EventTarget methods) are all reachable via the exported global. - Subprocess tests: pipes drained concurrently, no bare
stderr === "", combined{stdout, exitCode}assertions;onresourcetimingbufferfulltest restores the accessor infinally.
Extended reasoning...
Overview
Two files: src/js/node/perf_hooks.ts replaces the hand-written facade object with the global performance itself, and defines the three node-only members (nodeTiming, eventLoopUtilization, timerify) on Performance.prototype (non-enumerable, matching Node v26.3.0). Module-level eventLoopUtilization/timerify exports are read off the same prototype with a local fallback. test/js/node/perf_hooks/perf_hooks.test.ts adds ~100 lines covering object identity, prototype placement/enumerability, resource-timing method presence, the onresourcetimingbufferfull accessor, module-level export identity, and a fresh-subprocess test pinning the lazy-install contract.
Security risks
None identified. The descriptor literals carry __proto__: null, guards use Object.hasOwn, and the define target is the captured Performance.prototype (not getPrototypeOf of a replaceable global) — all hardened per earlier review rounds. No user input reaches a syscall, path, or eval.
Level of scrutiny
Medium. This is a Node-compat API-shape change, not a hot path or memory-safety change. But it does mutate a WebCore global's prototype from src/js/node/ as a first-require side effect — before this PR the web Performance.prototype was untouched. The PR body argues this is strictly closer to Node (where these live on the prototype) and that a native C++ move is the follow-up; that reasoning is sound, but whether Bun wants node-only members on the web global's prototype (visible to code that never imported node:perf_hooks, once anything else in the process has) is the kind of layering call REVIEW.md's "Never change a Bun-native default to fix Node compatibility" rule is adjacent to. It's defensible either way, so a maintainer should ratify it rather than a bot.
Other factors
This PR has been through five review iterations; every inline comment (tamper-resistance, forward-compat export identity, comment length, subprocess-stderr pattern, guarded prototype dereference) was applied and is visible in the current diff. CI build 75149 passed on all lanes that ran perf_hooks; the three red lanes are documented main breaks unrelated to this diff. Tests are well-structured, fail on main, and follow harness conventions. The bug-hunting system found nothing in this pass.
`require("node:perf_hooks").performance` was a hand-written facade that
forwarded a hardcoded subset of the global `performance` object, so it drifted
from what the global actually carries: `markResourceTiming` was never forwarded,
`clearResourceTimings` was a local no-op shadowing the real method,
`onresourcetimingbufferfull` was a load-time value snapshot rather than live, and
the EventTarget methods were missing. It was also a different object than
`globalThis.performance`, unlike node where the two are identical.
Export the global `performance` directly. The only members node's performance
carries that the web `Performance` lacks are `nodeTiming` and
`eventLoopUtilization`; define those on the Performance prototype
(non-enumerable, matching node), so the module object can be the global itself
with no per-method forwarding. `eventLoopUtilization` is also exported at the
module level, sharing the same function identity, as node's lib/perf_hooks.js
does.
The in-process test file already has node:perf_hooks loaded, so it cannot
see the bare web global. Spawn a subprocess that observes the node-only
members absent on Performance.prototype before require("node:perf_hooks")
and present (non-enumerable) afterward, making the documented attach point
a CI-enforced contract.
9b34a71 to
491bf6b
Compare
There was a problem hiding this comment.
LGTM — the facade → global-object switch is what dylan-conway asked for, and every review round since has converged.
What was reviewed:
- Prototype-install block:
Performance?.prototypeguard,hasOwngates,__proto__: nulldescriptors, and thePerformancePrototype?.x ?? localexport fallbacks are all consistent now. - Confirmed the deleted facade's members (
markResourceTiming, realclearResourceTimings, liveonresourcetimingbufferfullaccessor) are all reachable via the exported global and covered by the new tests. - Subprocess tests follow house style (concurrent pipe drain, guarded stderr, combined
{stdout, exitCode}assertion); the fresh-process test pins the lazy-install contract. - CI build 75149: perf_hooks green on every lane; the 3 red jobs are unrelated main breaks (bake deinit segfault,
--no-orphansperl timeout).
Extended reasoning...
Overview
Two files: src/js/node/perf_hooks.ts drops the hand-rolled facade object (which forwarded a hardcoded, drifted subset of performance methods) and instead exports globalThis.performance directly, after installing the three node-only members (nodeTiming, eventLoopUtilization, timerify) onto Performance.prototype non-enumerable — exactly matching Node v26.3.0's placement. test/js/node/perf_hooks/perf_hooks.test.ts gains 7 new tests covering object identity, prototype placement/enumerability, resource-timing method reachability, the onresourcetimingbufferfull accessor, module-level export identity, and a fresh-subprocess lazy-install contract.
Security risks
None. This is a pure Node-compat shape change in a built-in JS module. The prototype-install block was hardened over several review rounds against prototype pollution and replaced-global scenarios: Performance.prototype (not getPrototypeOf(performance)) so a shimmed global can't redirect the define onto Object.prototype; Object.hasOwn guards so a polluted Object.prototype can't suppress the define; __proto__: null on every descriptor; Performance?.prototype + if (PerformancePrototype) so a pre-require delete globalThis.Performance degrades rather than throws. No auth, crypto, filesystem, or network surface.
Level of scrutiny
Medium. It mutates a global prototype from a node: module, which is an observable process-wide side effect — but that is precisely Node's behavior, and the maintainer (dylan-conway) explicitly requested this design over the facade on 2026-07-07 ("yes please do that. that's the proper fix"). The one behavioral nuance vs Node — the members appear on the prototype only after first require('node:perf_hooks') rather than eagerly — is documented in the PR body, pinned by a fresh-subprocess test, and is strictly closer to Node than the pre-PR state (where they weren't on the global at all).
Other factors
This PR has been through 7 iterations. Every inline finding I raised in prior runs (tamper-resistance of the define target, hasOwn vs in, module-level export identity under a future native move, 7-line comment, bare expect(stderr).toBe(""), reading exports off the replaceable global, unguarded Performance.prototype dereference) is resolved in the current diff and marked resolved on the thread. CodeRabbit's fresh-process-coverage request is also addressed. The bug-hunting system found nothing this round. CI on the final commit is green for the touched test file across all lanes; the three failing jobs are pre-existing main breaks with no perf_hooks in scope. #31825's timerify implementation is preserved verbatim; only its attachment point moved from the facade to the prototype.
|
landed in #34518, perf_hooks.performance is the global on main |
What
require("node:perf_hooks").performanceis now the globalperformanceobject itself, as in Node, instead of a separate wrapper.Background
The module exported a hand-written facade that forwarded a hardcoded subset of the global
performanceto it. That subset had drifted from what the global actually carries:markResourceTimingwas never forwarded.clearResourceTimingswas a localfunction () {}no-op, shadowing the real method.onresourcetimingbufferfullwas a value snapshotted at module load, so assigning to it never reached the global.And because it was a distinct object,
require("node:perf_hooks").performance !== globalThis.performance, unlike Node where the two are the same object.Fix
Export the global
performancedirectly. The only members Node'sperformancecarries that the webPerformancelacks arenodeTiming,eventLoopUtilization, andtimerify(the last from #31825, which has since merged); define those on thePerformanceprototype, non-enumerable, exactly as Node does (verified againstnode -eon v26.3.0: all three arePROTO,enumerable=false, andObject.keys(performance)is unchanged). With the node-only members on the prototype, the module object can be the global itself, and every per-method forwarder is gone.eventLoopUtilizationandtimerifyare also exported at the module level, read offperformanceafter the prototype install so they share function identity withperformance.<name>, matching Node'slib/perf_hooks.jsmodule.exports.One implementation note
The three node-only members are attached to the prototype from the
node:layer (src/js/node/perf_hooks.ts) rather than ported intoJSPerformance.cpp:nodeTimingis a fakedPerformanceEntry(it has to beinstanceof PerformanceEntrywith live getters),eventLoopUtilizationis the zeroed stub, andtimerifyis the full JS implementation from async_hooks,events,http,http2,perf_hooks: port Node.js async compatibility tests and fix the gaps they surface — ALS run/disable + withScope/defaultValue, http client ALS across reused agent sockets, http2 ALS context, AsyncResource.bind, EventEmitterAsyncResource, timerify (+22 tests) #31825; porting them to C++ now would mean reimplementing that work. perf_hooks: implement performance.eventLoopUtilization() #32618 is turningeventLoopUtilizationinto a real implementation in this same JS layer.if (!Object.hasOwn(prototype, name))guards targetPerformance.prototype(neverObject.getPrototypeOf(performance), so a replaced global can't redirect the define ontoObject.prototype) and make the install forward-compatible: if any of these later move intoJSPerformancein C++, the JS definitions are skipped rather than clobbering them.The one behavioral nuance versus Node: the members attach when
node:perf_hooksis first required, rather than being present on a bareglobalThis.performancebefore anyrequire. They are node-only and weren't on the global at all before this change, so this is strictly closer to Node; making them eager is a follow-up that belongs with the C++ move.Merge-conflict resolution against #31825
#31825 added
timerify(and itsprocessTimerifyCompletehelper, plus thePerformanceNodeEntryclass andenqueueNodeEntry) on the facade and as a module-level export. All of that is kept unchanged here; the only change relative to #31825 is wheretimerifylives onperformance: onPerformance.prototypealongsidenodeTiming/eventLoopUtilization, instead of on a separate facade object. Thetimerifytests,PerformanceNodeEntryshape checks, and prototype-pollution tests from #31825 all still pass.Verification
bun bd test test/js/node/perf_hooks/perf_hooks.test.ts→ 13 pass. Withsrc/js/node/perf_hooks.tsreverted to main, 4 of the 13 fail (the single-object assertions; #31825'stimerifytests still pass against main as expected).Node parity confirmed side by side on v26.3.0:
perf_hooks.performance === globalThis.performance,nodeTiming/eventLoopUtilization/timerifypresent and on the prototype non-enumerable, module-leveleventLoopUtilization/timerifywith matching identity.test-perf-hooks-timerify-histogram-async.mjs,test-performance-function-async.js,test-net-perf_hooks.js,test-performance-measure.jsandtest/js/web/timers/performance.test.jsall pass.[review] gate passed · iteration 9 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 1 rejected · iteration 9
evidence per changed file