perf_hooks: export the real performance object and fix entry prototypes (+5 tests) - #34518
Conversation
perf_hooks.performance was a hand-written object literal that forwarded to the
global instead of being it, so globalThis.performance !== require('perf_hooks')
.performance and the Node-only surface (timerify, nodeTiming,
eventLoopUtilization, the EventTarget methods) was missing or duplicated. Export
the real global and install the Node-only members on Performance.prototype as
non-enumerable properties, matching lib/internal/perf/performance.js.
PerformanceNodeTiming used $toClass, which installs a brand new empty prototype
and so dropped every accessor and method declared on the class; lookups then
fell through to PerformanceEntry's brand-checking getters and threw. Link the
prototype chain directly instead. PerformanceObserver's node-types subclass had
the same problem and only needs its public name fixed.
PerformanceResourceTiming was exported as a throwNotImplemented stub that
shadowed the working native class.
Entries now carry util.inspect.custom so they print as `<ClassName> { ... }`
rather than `{}`, and mark/measure include detail in toJSON. The inspector uses
the same circular check util.inspect uses to skip custom inspectors on prototype
objects, since the accessors it reads are brand-checked.
measure(name, options, endMark) dropped endMark whenever the options dictionary
carried no start or end, measuring to now() instead of to the mark. Node ignores
such a dictionary for timing but still honours the trailing endMark and keeps
detail, so route that case through the endMark while clearing duration.
getEntriesByType/getEntriesByName now report Node's ERR_MISSING_ARGS wording,
and mark/clearMarks match Node's message for a Symbol argument.
Adds 5 upstream Node v26.3.0 tests, copied verbatim, plus common.sleepSync
(also verbatim from upstream) which one of them imports.
|
Updated 1:08 AM PT - Jul 19th, 2026
❌ @cirospaciari, your commit 22b294c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34518That installs a local version of the PR into your bun-34518 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
@robobun adopt |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
Comment |
|
Merged in 97935c2. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
clearMarks and clearMeasures are exact siblings in Node (both coerce the name via template literal), so clearMeasures(Symbol()) should throw the same V8 wording that clearMarks(Symbol()) does. The guard was added to mark/clearMarks but missed clearMeasures. measure() is deliberately excluded: Node validates its name arg via validateString and throws ERR_INVALID_ARG_TYPE instead.
…k path
Pins the two load-bearing lines in PerformanceUserTiming::measure's
options-dict visitor: the !endMark.isNull() block (e3 empty dict case)
and the duration = std::nullopt clear (e2 {duration: 999} case).
Verified byte-identical against Node v26.3.0.
There was a problem hiding this comment.
No bugs found this pass; both earlier nits (clearMeasures Symbol guard, trailing-endMark test coverage) are addressed. Deferring to a human because this makes design choices worth a maintainer sign-off: mutating Performance.prototype / PerformanceEntry.prototype at first require('perf_hooks') (with the acknowledged bootstrap-timing limitation), and diverging isNonEmptyDictionary from User Timing L3 in favour of Node semantics.
What was reviewed:
perf_hooks.ts: realperformanceexport, prototype-chain wiring replacing$toClass, custom inspect /toJSONfor entries — checked the prototype-object guard in the inspector and thatthrowNotImplementedhas no remaining callers.PerformanceUserTiming.cpp: verified the reshaped visitor still rejects{start,duration,end}and{start|end}+endMark, and thatdurationis cleared before recursing.JSPerformance.cpp: Symbol guards now cover mark/clearMarks/clearMeasures;measurecorrectly excluded.
Extended reasoning...
Overview
Rewrites node:perf_hooks to export the real global performance object instead of a forwarding literal, moving timerify/nodeTiming/eventLoopUtilization onto Performance.prototype as non-enumerable properties. Replaces $toClass(PerformanceNodeTiming, …) (which was dropping the class's own prototype methods) with direct setPrototypeOf wiring. Adds a nodejs.util.inspect.custom on PerformanceEntry.prototype and toJSON overrides on PerformanceMark/PerformanceMeasure so entries print with detail. In C++: switches getEntriesByType/getEntriesByName no-arg errors to ERR_MISSING_ARGS, adds V8-worded Symbol guards to mark/clearMarks/clearMeasures, and changes PerformanceUserTiming::measure so an options dict without start/end falls through (honouring a trailing endMark and keeping detail) instead of throwing per User Timing L3. Adds 5 upstream Node v26.3.0 tests plus local coverage.
Security risks
None identified. No auth/crypto/permissions/untrusted-input parsing. The prototype mutations use __proto__: null descriptors and are on WebCore-owned prototypes; the custom inspector guards against running on prototype objects.
Level of scrutiny
Medium-high. This is Node-compat surface, not a hot path, but it (a) mutates global prototypes process-wide when the module is first required — with an acknowledged limitation that behaviour depends on load order until a follow-up moves it to bootstrap; (b) deliberately diverges the WebCore measure() argument validation from the W3C spec toward Node's semantics; and (c) overlaps with six flagged open PRs (#27887, #27921, #32163, #32481, #32618, #33475). Those are architectural / roadmap calls a maintainer should own.
Other factors
Both prior review findings were addressed with tests. Test coverage is solid: the new perf_hooks.test.ts cases pin the Symbol wording across all three siblings and the three {detail}/{duration}/{} + endMark shapes, and five vendored Node tests exercise the exported-object identity, timeline ordering, ERR_MISSING_ARGS codes, and detail inspection. The common/index.js sleepSync addition mirrors upstream Node's helper. cirospaciari has already adopted the PR, so a maintainer is engaged; given the design surface here I'd rather they approve than auto-approve.
The body now checks only start||end, so the old name no longer described the invariant. File-local static with one caller.
a0666a1 to
842adba
Compare
There was a problem hiding this comment.
No bugs found after four review rounds (all nits addressed). Deferring to a human because this patches global prototypes (Performance.prototype, PerformanceEntry.prototype, PerformanceMark/Measure.prototype.toJSON) at module-load time and shifts PerformanceUserTiming::measure away from the User Timing L3 spec toward Node semantics — worth a maintainer's eyes on the design tradeoff, especially the acknowledged "installs on first require, not bootstrap" limitation.
What was reviewed:
- Symbol-argument guards on
mark/clearMarks/clearMeasures(sibling coverage now complete;measure()correctly excluded). measure(name, {detail|duration}, endMark)path — trailing endMark now honoured,durationcleared; test pins both clauses.- Checked that
measure(name, {duration: N})(2-arg) still works — falls through to the options-only overload wheredurationalone is ignored (start=0, end=now()), matching Node; not a regression. hasStartOrEndrename and comment-length trims verified in place.
Extended reasoning...
Overview
Restructures node:perf_hooks to export the real global performance object instead of a forwarding shim, moving Node-only members (timerify, nodeTiming, eventLoopUtilization) onto Performance.prototype as non-enumerable properties. Also installs a custom-inspect symbol on PerformanceEntry.prototype and overrides toJSON on PerformanceMark/PerformanceMeasure prototypes so entries print like Node's. Replaces the $toClass call on PerformanceNodeTiming (which was emptying the prototype) with direct setPrototypeOf. On the C++ side: swaps two createNotEnoughArgumentsError sites for ERR_MISSING_ARGS, adds V8-worded Symbol guards to mark/clearMarks/clearMeasures, and reworks the measure(name, options, endMark) visitor so a {detail}/{duration}-only dict falls through and honours the trailing endMark (Node's behaviour, diverging from User Timing L3). Adds 5 vendored Node v26.3.0 tests plus local coverage in perf_hooks.test.ts, and adds sleepSync to the vendored test-common harness.
Security risks
None identified. No untrusted-input parsing beyond existing IDL conversion; the new prototype patches use __proto__: null descriptors; the custom inspector guards against being invoked on prototype objects. No auth/crypto/permissions surface.
Level of scrutiny
Medium-high. The runtime change is Node-compat surface, not a hot path or memory-safety-sensitive code, but it mutates global prototypes at first-require time — anything that touched PerformanceEntry.prototype[Symbol.for('nodejs.util.inspect.custom')] or PerformanceMark.prototype.toJSON before/after this module loads now sees different behaviour. The C++ change deliberately diverges from the WebCore/spec path for Node compat, which is an architecture-layer decision ("never change a Bun-native default to fix Node compatibility — that belongs in the node: compat layer"; here it's done in the shared WebCore layer, though performance is only ever the one global so the distinction is thin). The PR description explicitly flags a known limitation (patches install on first require, not bootstrap) as follow-up. These are design calls a maintainer should sign off on rather than a bot.
Other factors
I left four rounds of inline nits on earlier revisions (missing clearMeasures sibling guard, unexercised !endMark.isNull() branch, misleading isNonEmptyDictionary name, >3-line comments); all four were addressed with follow-up commits and are verified present in the current diff. The bug-hunting system found no issues this run; one candidate — "measure(name, {duration: N}) no longer throws TypeError, duration silently ignored" — was examined and ruled out: with no start/end and no third arg, the fall-through measure(globalObject, measureName, measureOptions) computes start=0, end=now() and never reads duration, which matches Node v26.3.0 (Node's calculateStartDuration also ignores a lone duration). A maintainer has already adopted the PR, and CI build #75110 is running.
|
@robobun adopt it |
There was a problem hiding this comment.
No new findings; all four earlier review threads are resolved. Deferring to a maintainer given the scope — this restructures node:perf_hooks to mutate global Performance/PerformanceEntry/PerformanceMark/PerformanceMeasure prototypes on first require, and diverges PerformanceUserTiming::measure from the User Timing L3 spec toward Node's semantics.
What was reviewed:
measure(name, {duration: N})no longer throwing TypeError — matches Node'scalculateStartDuration, ruled out as intentional.- Symbol-argument guards now cover all three siblings (
mark/clearMarks/clearMeasures);measurecorrectly excluded. - The new
!endMark.isNull()branch andduration = std::nulloptclear are pinned by the addedperf_hooks.test.tscases. - Custom-inspect on
PerformanceEntry.prototypeguards against prototype-object receivers soconsole.log(PerformanceEntry.prototype)doesn't throw.
Extended reasoning...
Overview
This PR replaces the hand-written perf_hooks.performance forwarding object with the real global performance, and moves Node-only members (timerify, nodeTiming, eventLoopUtilization) onto Performance.prototype as non-enumerable properties. It also installs a nodejs.util.inspect.custom handler on PerformanceEntry.prototype and toJSON overrides on PerformanceMark/PerformanceMeasure prototypes, replaces $toClass with direct Object.setPrototypeOf for PerformanceNodeTiming, and swaps the PerformanceResourceTiming stub for the real global class. On the C++ side it changes getEntriesByType/getEntriesByName to throw ERR_MISSING_ARGS, adds V8-worded Symbol guards to mark/clearMarks/clearMeasures, and reworks PerformanceUserTiming::measure so an options dict without start/end falls through and honours a trailing endMark (renaming isNonEmptyDictionary → hasStartOrEnd). Five upstream Node v26.3.0 tests are vendored, plus new cases in perf_hooks.test.ts and a sleepSync helper in the test common harness.
Security risks
None identified. No auth/crypto/permissions surface. The prototype-pollution-safe patterns (__proto__: null in descriptors, kEmptyObject defaults) are preserved. The custom-inspect handler reads this.constructor and calls this.toJSON(), which are user-overridable, but that matches Node's own performance_entry.js behavior and is not a security boundary.
Level of scrutiny
Medium-high. This is Node-compat work with good test coverage (5 vendored upstream tests + targeted local tests, all verified against Node v26.3.0), but it makes several design decisions a maintainer should sign off on: (1) side-effecting global prototype mutation on require('perf_hooks') — the PR description flags this as a known limitation vs. installing at bootstrap; (2) deliberately diverging PerformanceUserTiming.cpp from the User Timing L3 spec toward Node behavior in shared WebCore code; (3) six potentially-overlapping open PRs were flagged by the duplicate-PR bot. None of these are bugs, but they're the kind of choices REVIEW.md's Architecture section says need maintainer agreement.
Other factors
I left four inline comments on earlier revisions (missing clearMeasures sibling, untested endMark branch, misleading isNonEmptyDictionary name, comment-length rule) — all four were addressed and the threads are resolved. The bug-hunting system found no issues this run; one candidate (measure(name, {duration: N}) no longer throwing) was examined and ruled out as the intended Node-compat behavior. CI build #75110 was reported green on 253 lanes with only agent-loss/unrelated flakes. The maintainer is actively engaged (two "adopt" pings), so this is ready for their review rather than mine to approve.
…v8 flag validation (+3 tests) (#34523) Three independent Node v26.3.0 compatibility gaps, each shipped with the upstream test that covers it, copied verbatim. None of them touch `perf_hooks`. Selected the same way as #34517: diffed Node v26.3.0's `test/parallel` + `test/sequential` against main **and** against the vendored tests of all 32 open PRs, then ran the 956 missing tests through the runner's exact invocation. ## `net` — connect errors lost the `- Local (…)` suffix `ExceptionWithHostPort` dropped Node's fifth `additional` argument (`lib/internal/errors.js:765-786`), which appends ` - Local (address:port)`. Both connect-failure paths in `net.ts` already computed that string and threw it away. ``` node: connect ECONNREFUSED 127.0.0.1:12399 - Local (127.0.0.1:12400) bun: connect ECONNREFUSED 127.0.0.1:12399 ``` Test: `sequential/test-net-connect-local-error` ## `console` — per-stream `inspectOptions` was ignored Node v26 lets `new Console({ inspectOptions })` take a `Map` keyed by stream so stdout and stderr can be formatted differently (`lib/internal/console/constructor.js:144-157, 334-335`). Bun stored the value raw and looked it up unkeyed, so a Map was treated as an options object with no `colors` and per-stream colors silently did nothing. Looks the options up per stream instead; the plain-object form still applies to both. Implemented with `$get` and **without** constructing a `Map`, so unlike a direct port of Node's code a tampered global `Map` cannot influence the result — verified against two of the three tamper repros review raised (the third, `_times`/`kCounts` at `ConsoleObject.ts:414,416`, is pre-existing and untouched). Test: `test-console-tty-colors-per-stream` ## `v8.setFlagsFromString` — validation ran after the not-implemented throw Node rejects a non-string argument with `ERR_INVALID_ARG_TYPE`; Bun threw `ERR_NOT_IMPLEMENTED` first. Now it validates first. Deliberately **not** turned into a no-op that records flags, which is what would be needed to also pass `test-v8-version-tag` — that would tell callers a flag applied when it didn't. `v8.setFlagsFromString('--allow_natives_syntax')` still reports `ERR_NOT_IMPLEMENTED`. Test: `test-v8-flag-type-check` ## Verification Driven against the real Node v26.3.0 binary: console inspect output, the connect error message and the v8 validation errors are **byte-identical** across ten checks, including per-stream colors, a Map with no entry for the stream being written to, and the `colorMode` conflict in both the Map and plain-object forms. - the 3 new tests: 3/3 green each under the runner's exact invocation - no regressions: 249/251 vendored `test-net-*` / `test-dgram-*` / `test-v8-*` / `test-console-*` tests pass; the 2 failures (`test-net-connect-keepalive`, `test-net-server-keepalive`) were confirmed pre-existing by running them on a build without this change - `test/js/node/net` + `test/js/node/dgram` is 208 tests / 0 fail, `test/js/node/console` 9 / 0 fail ## Noted, not fixed `net.ts:2892` and `:3022` (the synchronous connect-failure paths) also take a `details` argument in Node, computed from `self._getsockname()`. Bun never computes it there, so those paths still lose the suffix. Pre-existing and not reachable from this test; left for a follow-up. Independent of #34517 (14 vendored tests) and #34518 (perf_hooks).
3 participants
perf_hooks.performancewas a hand-written object literal that forwarded to the global instead of being it, soglobalThis.performance !== require('perf_hooks').performanceand the Node-only surface (timerify,nodeTiming,eventLoopUtilization, theEventTargetmethods) was missing or duplicated. This exports the real global and fixes the entry prototypes around it.Adds 5 upstream Node v26.3.0 tests, copied verbatim:
test-perf-hooks-timerify-basic,test-performance-global,test-performance-measure-detail,test-performance-timeline,test-perf-hooks-timerify-histogram-sync.What changed
performance. Node-only members move ontoPerformance.prototypeas non-enumerable properties, matchinglib/internal/perf/performance.js.Object.keys(performance).lengthgoes 15 → 2 (Node is 0).$toClasswas silently emptying class prototypes. It installs a brand-new empty prototype, soPerformanceNodeTiming'sget name(),get entryType()andtoJSON()were dropped and lookups fell through toPerformanceEntry's brand-checking getters and threw. Now the prototype chain is linked directly.PerformanceObserver's node-types subclass had the same bug and only needed its publicnamefixed.PerformanceResourceTimingwas exported as athrowNotImplementedstub shadowing the working native class.toJSON. Entries print asPerformanceMark { … }instead of{}, and mark/measure includedetail.measure(name, options, endMark)droppedendMarkwhenever the options dictionary carried nostart/end, measuring tonow()instead of to the mark. Node ignores such a dictionary for timing but still honours the trailingendMarkand keepsdetail.getEntriesByType/getEntriesByNameuse Node'sERR_MISSING_ARGSwording;mark/clearMarksmatch Node's message for a Symbol argument.Verification
Driven against the real Node v26.3.0 binary with a script exercising every touched surface; output is identical except for two pre-existing divergences this PR doesn't address (
Object.keys(performance).lengthis 2 vs Node's 0, and a destructuredperformance.now()doesn't throw the brand-checkTypeErrorNode throws).test-dns-*/test-perf-hooks-*/test-performance-*tests pass, andtest/js/node/perf_hooks+test/js/bun/perf_hooks+test/js/node/dns+test/js/web/timersis 174 pass / 0 failTwo bugs found in review and fixed before submitting, both confirmed against Node first:
console.log(PerformanceEntry.prototype)threw aTypeErrorbecause the new custom inspector ran on prototype objects (prototype inspection is now byte-identical with and without this patch), and theendMarkbug above.Known limitation
The prototype patches install when
node:perf_hooksis first required, not at bootstrap, soutil.inspect(entry)output andperformance.timerifystill depend on whether something in the process loadedperf_hooks. Node has them from the start. Installing at global bootstrap is the proper fix and is left as follow-up.Deliberately not included
test-performance-gcneeds aJSC::HeapObserverand agcentry type that doesn't exist.test-performance-nodetimingandtest-perf-hooks-usertimingneed real startup milestones — Bun's are fabricated constants set totimeOrigin— so they can only be made to pass by faking the values.Independent of #34517, which vendors 14 tests needing no source change.
[review] gate passed · iteration 2 · 11 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 2
evidence per changed file