perf_hooks: alias globalThis.PerformanceObserver to the node:perf_hooks class - #33789
Open
robobun wants to merge 4 commits into
Open
perf_hooks: alias globalThis.PerformanceObserver to the node:perf_hooks class#33789robobun wants to merge 4 commits into
robobun wants to merge 4 commits into
Conversation
…ks class
The global PerformanceObserver was the raw WebCore class, while
node:perf_hooks exports a subclass that routes node-only entry types
(http/net/dns) through a JS-side registry. Instrumentation written
against the global accepted observe({entryTypes:['http','net']})
without error but never delivered any entries; only the perf_hooks
export did.
The global is now a lazy custom accessor that returns the perf_hooks
subclass, so globalThis.PerformanceObserver ===
require('node:perf_hooks').PerformanceObserver with a single
supportedEntryTypes list. perf_hooks.ts fetches the native base
constructor directly via a C++ hook to avoid re-entering itself on
first access. A static-table PropertyCallback cannot be used for the
global because abstractResolve() reifies it under a VMInquiry slot
that forbids VM re-entry.
Collaborator
Author
|
Updated 7:17 PM PT - Jul 8th, 2026
❌ @robobun, your commit ad5d5ad has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33789That installs a local version of the PR into your bun-33789 --bun |
Contributor
WalkthroughThis PR changes how ChangesPerformanceObserver identity unification
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/js/node/perf_hooks.ts`:
- Around line 18-21: The comment for PerformanceObserverForNodeTypes is stale
and still describes NodePerformanceObserver as an alias destructured from
globalThis, which no longer matches the $cpp-based lookup used in perf_hooks.ts.
Update the documentation near PerformanceObserverForNodeTypes to reflect that
NodePerformanceObserver is obtained directly from JSPerformanceObserver.cpp via
getPerformanceObserverConstructor, and remove any mention of the old globalThis
destructuring. Keep the wording aligned with the current NodePerformanceObserver
initialization so future readers understand the native constructor is fetched
directly.
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 1196-1208: The getPerformanceObserverGlobal getter is
dereferencing perfHooks.getObject() without verifying the requireId() result is
actually an object, which can lead to a null-deref crash. Add the same object
guard pattern used elsewhere in ZigGlobalObject.cpp (for example, the
m_utilInspectFunction.initLater node:util path) before calling getObject(), and
fail loudly with an assertion or equivalent check before accessing
PerformanceObserver.
- Around line 3004-3005: The PerformanceObserver global is currently defined as
a getter-only accessor in ZigGlobalObject::putDirectCustomAccessor, which makes
assignment fail in strict mode until the getter is touched. Update the
PerformanceObserver binding in ZigGlobalObject to keep it writable by adding a
setter or converting it to a data property, while preserving the existing getter
behavior and property setup.
In `@test/js/node/perf_hooks/perf_hooks.test.ts`:
- Around line 92-101: The assertion block in the perf_hooks test should stop
requiring stderr to be exactly empty and should validate the process output more
robustly. Update the test around the Promise.all result so it checks the
combined stdout, stderr, and exitCode together instead of expecting empty
stderr, and in the JSON parsing path guard against empty or failed stdout by
surfacing stderr/exitCode when parsing cannot succeed. Keep the fix localized to
the test case using the existing proc.stdout.text(), proc.stderr.text(), and
proc.exited flow.
🪄 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: 3f001ea1-7df5-4149-b289-5bbd73ee5d56
📒 Files selected for processing (6)
src/js/node/perf_hooks.tssrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.lut.txtsrc/jsc/bindings/webcore/JSPerformanceObserver.cppsrc/jsc/bindings/webcore/JSPerformanceObserver.htest/js/node/perf_hooks/perf_hooks.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/bindings/ZigGlobalObject.lut.txt
Contributor
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/jsc/bindings/ZigGlobalObject.cpp (1)
3005-3005: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
PerformanceObservernon-enumerable onglobalThis.Both the accessor and the cached own property use
0attributes, soPerformanceObserverwill show up inObject.keys(globalThis)/for..inafter first access. Mark both sitesDontEnumto match the other global constructors.🤖 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/jsc/bindings/ZigGlobalObject.cpp` at line 3005, The PerformanceObserver global accessor is currently defined with enumerable attributes, so it can appear in Object.keys(globalThis) and for..in after first access. Update the global property setup in ZigGlobalObject’s PerformanceObserver binding to use DontEnum for both the accessor and the cached own property, matching the other global constructors and keeping the property non-enumerable.
🤖 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/jsc/bindings/ZigGlobalObject.cpp`:
- Line 3005: The PerformanceObserver global accessor is currently defined with
enumerable attributes, so it can appear in Object.keys(globalThis) and for..in
after first access. Update the global property setup in ZigGlobalObject’s
PerformanceObserver binding to use DontEnum for both the accessor and the cached
own property, matching the other global constructors and keeping the property
non-enumerable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb7f43b1-e0ec-4e2d-92b5-e1e458dd86cf
📒 Files selected for processing (3)
src/js/node/perf_hooks.tssrc/jsc/bindings/ZigGlobalObject.cpptest/js/node/perf_hooks/perf_hooks.test.ts
Collaborator
Author
|
Re: making This preserves the existing Bun behavior. The whole Flipping only |
Collaborator
Author
|
CI is red on unrelated lanes; the diff itself is green.
None of these touch Ready for a maintainer to merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
globalThis.PerformanceObserverwas the raw WebCore class, whilerequire('node:perf_hooks').PerformanceObserveris a subclass that routes node-only entry types (http/net/dns) through a JS-side registry. Instrumentation written against the global acceptedobserve({entryTypes:['http','net']})without error but never delivered any entries; only the perf_hooks export did. Node documents the two as the same object.Companion to #33475 (
perf_hooks.performance !== globalThis.performance); that was theperformanceobject split, this is the observer class/registry split.Fix
globalThis.PerformanceObserveris now a lazy custom accessor that resolves to thenode:perf_hookssubclass, so the two are the same object with onesupportedEntryTypeslist that includes the node types.Two mechanical pieces:
perf_hooks.tspreviously read the native base class offglobalThis; that would now re-enter the module load, so it fetches the native constructor directly viagetPerformanceObserverConstructorinJSPerformanceObserver.cpp(same shape asgetWebSocketConstructor).PropertyCallback:JSScope::abstractResolve()reifies those under aVMInquiryPropertySlotthat forbids VM re-entry, and loadingperf_hooksruns JS (crashes with acheckVMEntryPermissionabort on the first bare-identifier reference). It is installed viaputDirectCustomAccessorinstead, which only records the getter in the slot; the value is materialized on first runtime read and then cached withputDirect.Verification
New tests in
test/js/node/perf_hooks/perf_hooks.test.ts, both failing on main:globalThis.PerformanceObserver === perf.PerformanceObserverandsupportedEntryTypesincludeshttp/net/dns.node:perf_hooks, observes['http','net'], makes anhttp.request, and asserts the global observer receives the samehttp:HttpClient/http:HttpRequest/net:connectentries the perf_hooks observer does.Also pass:
performance-observer-leak.test.ts(Worker with an undisconnected observer, exercises the lazy accessor inside a worker VM),test/js/deno/performance/performance.test.ts, the vendoredtest-net-perf_hooks.js,test-http-perf_hooks.js,test-performance-measure.js, andweb-globals.test.js.[review] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file