perf_hooks: implement performance.eventLoopUtilization() - #32618
Conversation
|
Updated 12:13 AM PT - Jul 16th, 2026
❌ @robobun, your commit 1b4d69b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32618That installs a local version of the PR into your bun-32618 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements ChangesEvent Loop Utilization
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/bun-usockets/src/loop.c`:
- Around line 53-55: The code unconditionally calls
uv_metrics_idle_time(loop->uv_loop) without verifying that metrics are enabled
on the loop instance. Search the codebase to confirm that uv_loop_configure() is
being called with the UV_METRICS_IDLE_TIME flag on the same loop instance used
in the LIBUS_USE_LIBUV block. If this configuration call is not found, either
add uv_loop_configure(loop->uv_loop, UV_METRICS_IDLE_TIME) before reading the
metrics, or add clear comments documenting that loop metrics enablement is a
required prerequisite for this code to work correctly.
🪄 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: d7107156-733c-4c00-8091-b91d1e7f231b
📒 Files selected for processing (18)
packages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/internal/loop_data.hpackages/bun-usockets/src/loop.csrc/js/internal/shared.tssrc/js/node/perf_hooks.tssrc/js/node/worker_threads.tssrc/jsc/bindings/webcore/JSWorker.cppsrc/jsc/bindings/webcore/Worker.cppsrc/jsc/bindings/webcore/Worker.hsrc/jsc/event_loop.rssrc/jsc/web_worker.rssrc/runtime/dispatch_js2native.rssrc/uws/lib.rssrc/uws_sys/InternalLoopData.rssrc/uws_sys/Loop.rstest/js/node/perf_hooks/perf_hooks.test.tstest/js/node/worker_threads/worker_threads.test.ts
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 `@packages/bun-usockets/src/eventing/libuv.c`:
- Around line 160-163: The uv_loop_configure function call for
UV_METRICS_IDLE_TIME is not checking its return code, which means failures are
silently ignored and idle metrics never accumulate, leading to incorrect event
loop utilization reporting on the libuv path. Check the return code from the
uv_loop_configure call and add explicit error handling (such as logging a
warning or error message) to alert when metric configuration fails, ensuring
that failures do not go undetected.
🪄 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: 490c0519-8d68-460c-afd7-da2807c1a672
📒 Files selected for processing (1)
packages/bun-usockets/src/eventing/libuv.c
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/worker_threads/worker_threads.test.ts`:
- Around line 678-691: The test leaves the worker running indefinitely if any
assertion fails before termination, and relies on a fixed timing wait instead of
waiting for an observable condition. Refactor the test by wrapping the worker
logic in a try/finally block to guarantee worker cleanup, replace the
Bun.sleep(50) with a condition-driven wait that signals after the worker
completes its busy work (such as a message event handler or port communication),
and move the worker.terminate() call to the finally block to ensure it executes
regardless of assertion failures.
🪄 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: 65f56682-3642-4b6c-8b52-d585ff4a8530
📒 Files selected for processing (3)
src/js/node/worker_threads.tssrc/jsc/web_worker.rstest/js/node/worker_threads/worker_threads.test.ts
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 `@packages/bun-usockets/src/loop.c`:
- Around line 58-64: To ensure the reader obtains a consistent snapshot of both
idle_time_ns and idle_entry_ns, refactor the load sequence in the section
beginning with uint64_t idle_ns. Read idle_entry_ns first with ACQUIRE
semantics, then read idle_time_ns with RELAXED semantics, then read
idle_entry_ns again with ACQUIRE semantics. If the second reading of
idle_entry_ns differs from the first, retry the entire sequence. This prevents
the reader from observing a stale idle_time_ns value paired with a cleared
idle_entry_ns, which would cause the in-progress credit calculation to be
skipped incorrectly.
🪄 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: fceac5b6-bcce-472f-8987-6ce16d77b1e5
📒 Files selected for processing (5)
packages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/internal/loop_data.hpackages/bun-usockets/src/loop.csrc/uws_sys/InternalLoopData.rstest/js/node/worker_threads/worker_threads.test.ts
There was a problem hiding this comment.
All earlier review feedback has been addressed and I didn't find new issues, but this adds per-tick atomics to the core epoll/kqueue dispatch path and new cross-thread unsafe reads of worker loop state, so it's worth a human look before merging.
Extended reasoning...
Overview
This PR implements performance.eventLoopUtilization() and worker.performance.eventLoopUtilization(), replacing zeroed stubs. It spans 19 files across four layers: usockets C (idle-time accumulation in epoll_kqueue.c/loop.c/libuv.c, three new fields on us_internal_loop_data_t), Rust FFI (uws_sys, uws, event_loop.rs, web_worker.rs), C++ Worker bindings (JSWorker.cpp/Worker.cpp/Worker.h), and the JS layer (perf_hooks.ts, worker_threads.ts, shared.ts), plus tests.
Security risks
None apparent. The new surface is a read-only diagnostic metric; no user input flows into pointer arithmetic or allocation sizes. The cross-thread read of the worker's loop pointer is guarded by vm_lock and uses a pointer cached at publish time, so there's no obvious UAF or injection vector.
Level of scrutiny
High. The change instruments us_loop_run_bun_tick — every event-loop tick on POSIX now executes two additional __atomic_store_n and one __atomic_add_fetch around the epoll_pwait2/kevent64 syscall. The lock-free two-counter design (idle_time_ns + idle_entry_ns with RELEASE/ACQUIRE pairing) deliberately trades a sub-µs under-count window for avoiding a mutex; that reasoning is documented in comments and was iterated on across several review rounds, but it's the kind of concurrency design choice a maintainer should sign off on. The new unsafe Rust in web_worker.rs (cross-thread uws::loop_event_loop_utilization under vm_lock) and the InternalLoopData struct-layout mirror also warrant a second pair of eyes.
Other factors
The PR went through six rounds of bot review; every flagged issue (missing UV_METRICS_IDLE_TIME configure, event_loop_handle data race, negative deltas after worker exit, mid-wait misattribution, AArch64 store-reordering double-count, dead warnNotImplementedOnce, comment wording) was fixed and the threads resolved. The current revision passed the bug-hunting system with no findings. Test coverage looks adequate (busy/idle main-thread, busy/idle/post-exit worker). Given the breadth and the hot-path atomics, I'm deferring rather than approving.
There was a problem hiding this comment.
All my earlier findings have been addressed and this round is clean, but given the scope — atomics in the event-loop hot path, cross-thread reads of worker state under vm_lock, and platform-split C/Rust/C++ plumbing across 19 files — this one deserves human eyes before merging.
Extended reasoning...
Overview
This PR implements performance.eventLoopUtilization() and worker.performance.eventLoopUtilization(), replacing zeroed stubs. It spans 19 files across four layers: usockets C (idle-time accounting around epoll_pwait2/kevent64/libuv with atomic counters in us_internal_loop_data_t), Rust FFI (uws_sys, uws, event_loop.rs, web_worker.rs), C++ bindings (Worker.cpp/JSWorker.cpp), and the JS surface (perf_hooks.ts, worker_threads.ts, internal/shared.ts). New tests cover busy/idle main-thread sampling and busy/idle/message-driven/post-terminate worker sampling.
Security risks
None apparent. The new surface is a read-only diagnostic metric. The cross-thread worker read is guarded by vm_lock and dereferences only an immutable creation timestamp plus atomic counters via a cached loop pointer; no user-controlled input flows into native code.
Level of scrutiny
High. The change instruments us_loop_run_bun_tick — the per-tick hot path — with two atomic stores and a clock_gettime on every non-zero-timeout poll, and adds release/acquire-ordered cross-thread state that took several iterations to get right (libuv configure missing on Windows; data race on event_loop_handle via spawnSync; mid-wait samples mis-attributed to active; AArch64 store reordering allowing double-count; pending_wakeups staleness inverting the metric for message-driven workers). All were fixed, but the density of subtle concurrency issues found during review is itself a signal that a maintainer familiar with the loop and worker-teardown invariants should sign off.
Other factors
The most recent CI report in the thread (for 299f598) shows build-rust/build-cpp failures across most targets; the follow-up commit bc75bad adds a clippy allow on the FFI export and presumably clears them, but I don't see a green build confirmed in the timeline yet. The new unsafe Rust in web_worker.rs (caching uws_loop_ptr at the VM publish point and reading it under vm_lock) and the InternalLoopData layout extension both look correct to me, but layout/ABI changes to a struct mirrored in three places (C header, Rust #[repr(C)], and the runtime) are the kind of thing a human should double-check.
|
CI note for reviewers: the one persistent red lane is |
|
Note for reviewers: this branch's merge base is 147 commits behind main, and the For reference, an implementation of the main-thread |
bc75bad to
49370c7
Compare
There was a problem hiding this comment.
No new issues found — all prior feedback has been addressed and the $newRustFunction rebase is in place. Given the scope (per-tick instrumentation in us_loop_run_bun_tick, cross-thread lock-free atomics with release/acquire ordering, and worker-VM lifetime handling under vm_lock), this still warrants a maintainer's sign-off.
Extended reasoning...
Overview
This PR implements performance.eventLoopUtilization() and worker.performance.eventLoopUtilization() end-to-end across 19 files and four languages: usockets C (epoll_kqueue.c, libuv.c, loop.c, loop_data.h) instruments the event-provider wait with atomic idle-time counters and an in-progress-wait marker; Rust FFI (uws_sys/Loop.rs, uws_sys/InternalLoopData.rs, uws/lib.rs) mirrors the struct layout and exposes a sampler; event_loop.rs and web_worker.rs add the same-thread and cross-thread host functions (the latter caching the worker's uWS loop pointer at VM-publish time and reading it under vm_lock); C++ (Worker.{h,cpp}, JSWorker.cpp) adds the prototype method; and TypeScript (shared.ts, perf_hooks.ts, worker_threads.ts) computes the utilization ratio and diffs. Six new tests cover busy/idle main-thread, busy/idle/message-driven workers, and post-terminate zeroing.
Security risks
None identified. No user input parsing, no auth/crypto/permissions, no filesystem or network surface. The new code reads monotonic clocks and atomic counters. The cross-thread read is bounded by vm_lock and dereferences only an immutable timestamp plus atomically-written counters.
Level of scrutiny
High. This adds two clock_gettime calls plus three atomic stores to every event-loop tick on POSIX (us_loop_run_bun_tick is the hottest path in the runtime). It introduces lock-free cross-thread state with hand-tuned memory orderings that already went through one AArch64 correctness fix during review. It adds new unsafe Rust with SAFETY invariants tied to worker arena lifetime, and one of those SAFETY comments was found incorrect and reworked mid-review. It changes us_internal_loop_data_t layout (mirrored in Rust). None of this is "simple, mechanical, or obvious."
Other factors
The PR has been through eight rounds of automated review; every finding (missing UV_METRICS_IDLE_TIME config, event_loop_handle data race, mid-wait idle attribution, AArch64 store reordering, pending_wakeups gating, post-terminate negative deltas, dead-code cleanup, comment accuracy) was fixed with a targeted commit and a regression test where applicable. The current bug-hunting pass found nothing. Test coverage looks solid. That said, per the approval guidelines this is squarely "complex, large, touches critical code paths" — a human maintainer should confirm the per-tick overhead is acceptable and that the vm_lock / arena-lifetime reasoning in web_worker.rs holds.
1f85cc7 to
782227b
Compare
There was a problem hiding this comment.
No further issues found after the earlier rounds of fixes, but this instruments the core event-loop hot path (two clock reads + atomic stores per tick) and adds cross-thread sampling of a worker's loop under vm_lock — worth a maintainer's eyes on the hot-path cost and the concurrency design before merge.
Extended reasoning...
Overview
This PR implements performance.eventLoopUtilization() for both the main thread and worker.performance.eventLoopUtilization() for cross-thread sampling. It spans 19 files across four languages: C (usockets event-loop instrumentation in epoll_kqueue.c/loop.c/libuv.c, new fields in us_internal_loop_data_t), Rust (FFI in uws_sys/Loop.rs, InternalLoopData.rs mirror, web_worker.rs cross-thread sampler with vm_lock, event_loop.rs host fn), C++ (JSWorker/Worker bindings), and TypeScript (perf_hooks.ts, worker_threads.ts, shared helper in internal/shared.ts). It also removes the now-dead warnNotImplementedOnce helper and adds five new tests.
Security risks
None identified. The new surface is a read-only diagnostic metric; inputs are optional prior-snapshot objects whose numeric fields are only subtracted. No paths, no allocation sized from user input, no auth/crypto.
Level of scrutiny
High. The change instruments us_loop_run_bun_tick — the innermost per-tick hot path executed on every event-loop iteration — with two clock_gettime(CLOCK_MONOTONIC) calls and two atomic stores. It also introduces a lock-free two-field cross-thread read (idle_time_ns + idle_entry_ns) with hand-tuned acquire/release ordering, and a new parent→worker FFI path that dereferences a cached loop pointer under vm_lock. This class of change (hot-path instrumentation + cross-thread lock-free state on weak-memory targets) has already required multiple correctness fixes during review (Windows UV_METRICS_IDLE_TIME config, spawnSync data race on event_loop_handle, mid-wait sampling inversion, AArch64 store-reordering double-count, stale pending_wakeups gate on Linux). All were addressed, but the density of subtle issues found argues for a maintainer sign-off on the final design rather than bot approval.
Other factors
- The branch was rebased/updated after the
$newZigFunctionremoval on main (now uses$newRustFunction); the recent commits (dacd83d1,49f39736,432f143f,b124f568) are the mechanical follow-ups. - Test coverage is good: busy vs. idle main-thread, busy/idle/message-driven worker, and post-terminate zeros. Tests are timing-based by nature (asserting
> 50ms/< 0.5utilization over 200–300ms windows) which is inherent to the feature but worth a glance for CI flakiness headroom. - The one open design tradeoff explicitly left to maintainers (per the resolved thread on
loop.c) is whether the sub-µs under-count window between theidle_entry_nsclear and theidle_time_nscredit is acceptable vs. a seqlock — the author's reasoning for leaving it as-is seems sound for a monitoring metric. - CI note from the author flags an unrelated
grpc-jsSIGTRAP ondarwin-14-aarch64as pre-existing flake.
There was a problem hiding this comment.
No bugs found in the current revision, but this instruments the event-loop hot path with cross-thread lock-free atomics and threads a worker-VM pointer read through vm_lock — worth a human pass on the memory-ordering reasoning and the per-tick overhead before it lands.
Extended reasoning...
Overview
This PR implements performance.eventLoopUtilization() and worker.performance.eventLoopUtilization(), replacing zeroed stubs. It spans 19 files across five layers: usockets C (idle-time accumulation around epoll_pwait2/kevent64 in us_loop_run_bun_tick, plus a new us_loop_event_loop_utilization reader and libuv UV_METRICS_IDLE_TIME config), Rust FFI mirrors in uws_sys, a new Rust host function in event_loop.rs, a cross-thread FFI export on WebWorker that samples the worker's loop under vm_lock, C++ JSWorker prototype method + Worker::eventLoopUtilization, and shared JS logic in internal/shared.ts wired into perf_hooks.ts / worker_threads.ts. Tests cover busy/idle main-thread ELU and four worker scenarios (busy, post-terminate zeros, idle-blocked, message-driven idle).
Security risks
None identified. No user-controlled input reaches native parsing; the new surface reads two monotonic counters and returns three doubles. The cross-thread read path is guarded by vm_lock and touches only atomic/immutable fields of a cached loop pointer.
Level of scrutiny
High. The instrumentation lands directly in us_loop_run_bun_tick — every tick of every event loop now takes two extra clock_gettime calls and three atomic stores when the timeout is non-zero. The cross-thread reader is a hand-rolled lock-free two-field snapshot (release/acquire on idle_time_ns + idle_entry_ns) rather than libuv's mutex-guarded approach, with explicit reasoning about AArch64 store reordering. The WebWorker side caches a raw *mut uws::Loop at the VM publish point and dereferences it from the parent thread under vm_lock, relying on the arena-lifetime invariant that vm != null ⇒ loop is alive. These are exactly the kinds of decisions (per-tick hot-path cost, weak-memory correctness, cross-thread raw-pointer lifetime) that a maintainer should sign off on.
Other factors
The PR went through several rounds of substantive correction during automated review: missing UV_METRICS_IDLE_TIME on Windows, a data race on event_loop_handle (fixed by caching the loop pointer), mid-wait idle attribution (added idle_entry_ns), an AArch64 memory-ordering bug allowing double-count (upgraded to release/acquire), and a stale pending_wakeups gate that inverted the metric on Linux (decoupled to a timeout-only gate). All are now resolved and covered by regression tests, and the current bug-hunting pass found nothing. The branch has since been rebased onto the $newRustFunction codegen. Given the number of subtle concurrency issues already caught and fixed, and that this is a net-new feature touching core runtime infrastructure with no prior human review, deferring is the right call.
|
Verification status: this change is ready; the remaining red on the automated fail-before/pass-after check looks environmental rather than related to the diff. Evidence:
On CI: cargo clippy and all build lanes pass on this branch; the red test lanes so far have been unrelated flakes (grpc-js SIGTRAP on darwin-14, fetch-gzip on windows-2019-baseline, and a buildkite artifact-download timeout). |
There was a problem hiding this comment.
All prior review findings have been addressed and I found nothing new, but this touches the event-loop hot path with cross-thread atomics and new unsafe FFI, so it's worth a maintainer's eyes before merging.
Extended reasoning...
Overview
Implements performance.eventLoopUtilization() for both the main thread and worker.performance.eventLoopUtilization(). Spans 19 files across the stack: usockets C (idle-time accounting around epoll_pwait2/kevent64 in us_loop_run_bun_tick, plus uv_loop_configure(UV_METRICS_IDLE_TIME) on Windows), Rust FFI mirrors in uws_sys, a new extern "C" entry on WebWorker that reads a worker's loop counters from the parent thread under vm_lock, C++ Worker/JSWorker prototype additions, a shared JS diff helper, and tests.
Security risks
None identified. The change is diagnostic/metrics-only; no user input reaches parsing, allocation sizing, or filesystem/network paths. The cross-thread read is guarded by vm_lock and reads only atomic counters plus an immutable creation timestamp.
Level of scrutiny
High. This instruments the core event-loop tick on every iteration and adds lock-free concurrency (acquire/release ordering on idle_time_ns/idle_entry_ns) that is read cross-thread. During review it went through seven rounds of fixes for subtle issues — missing UV_METRICS_IDLE_TIME config, a data race on event_loop_handle during spawnSync, mid-wait sampling misattribution, AArch64 store reordering, and a stale-pending_wakeups gating bug. All were addressed, but the density of concurrency subtleties argues for a maintainer confirming the final shape (particularly the cached uws_loop_ptr lifetime under vm_lock and the two-field atomic read's accepted under-count window).
Other factors
- All prior inline findings (mine and CodeRabbit's) are marked resolved with corresponding commits.
- Tests cover busy/idle main-thread, busy/idle/message-driven workers, and the post-terminate zeros case; they fail on the released binary and pass with the fix.
- The unrelated timeout bumps to three pre-existing
worker_threadstests (execArgv,eval does not leak source code) are minor and justified in comments. - robobun's most recent notes indicate CI is green aside from unrelated flakes.
|
Agreed, this is ready for a maintainer pass. The concurrency pieces worth a close look are the acquire/release pairing on idle_time_ns/idle_entry_ns in epoll_kqueue.c and the cached uws loop pointer read under vm_lock in web_worker.rs; both are commented at the site. On CI: the only real failure in build 71528 (both lanes) is the fetch-gzip redirect test at fetch-gzip.test.ts:521 (InvalidHTTPResponse vs ConnectionRefused). It reproduces on a pristine merge-base build without this diff and is flaky on main, so it is unrelated. Retriggered once. |
|
Retrigger result (build 71532): the only remaining red lane is debian x64-asan, failing on the same unrelated test, fetch-gzip.test.ts:521 "followed redirect with a malformed chunked body rejects fetch()" (expects InvalidHTTPResponse, gets ConnectionRefused when the redirect connect to port 1 loses the race against the malformed-chunk parse). The Windows lane that hit it in the previous build is now green, and the test fails the same way on a pristine merge-base build without this diff. Everything this PR touches is green, including the new perf_hooks and worker_threads ELU tests. I'm not going to keep retriggering; this is ready for a maintainer. |
| /* Monotonic clock (ns) used for event-loop-utilization accounting. */ | ||
| uint64_t us_loop_monotonic_ns(void); | ||
| /* Sample the loop's accumulated idle time and active time (both ns). */ | ||
| void us_loop_event_loop_utilization(struct us_loop_t *loop, uint64_t *idle_ns_out, uint64_t *active_ns_out); |
There was a problem hiding this comment.
This seems wasteful and doesn't actually fix the issue at hand. Find another approach
There was a problem hiding this comment.
Want to make sure I rework this in the direction you have in mind, so a bit of context on both points:
On cost: the two clock reads happen only when the wait can actually block (NULL or nonzero timeout). Zero-timeout poll-throughs, i.e. every tick while the loop is busy, skip the instrumentation entirely, so the steady-state overhead is two vDSO clock_gettime calls per sleep/wake cycle, paid next to a syscall that already context-switches. This is the same measurement libuv performs under UV_METRICS_IDLE_TIME, which Node enables unconditionally for every loop including workers; the Windows path here just uses that.
On the issue: the reporter's repro from #32609 now produces Node's output and exits the same way. This branch prints { idle: 0, active: 168.4, utilization: 1 } and climbing, where Node v26 prints { idle: 0, active: 167.6, utilization: 1 }; the released Bun prints the NotImplementedError warning and zeros forever. So I may be missing what you mean by not fixing the issue at hand; if you saw a case where the numbers are wrong, I would like to chase it.
If the objection is that the accounting is always on, I can gate it behind a per-loop flag that flips on the first eventLoopUtilization() call, making the cost exactly zero until someone uses the API. The tradeoff is that idle time before the first call gets reported as active (Node reports absolute idle since loop start), though the common delta usage elu(prev) is unaffected. If you have a different mechanism in mind instead, point me at it and I will rework the PR.
There was a problem hiding this comment.
Sure. In libuv the measurement is the UV_METRICS_IDLE_TIME machinery:
- Right before the provider wait, gated on a nonzero timeout exactly like this PR:
src/unix/linux.c#L1453-L1465/* Only need to set the provider_entry_time if timeout != 0. ... */ if (timeout != 0) uv__metrics_set_provider_entry_time(loop); ... nfds = epoll_pwait(epollfd, events, ARRAY_SIZE(events), timeout, sigmask);
- The entry/exit clock reads and accumulation:
src/uv-common.c#L986-L1025(uv__metrics_set_provider_entry_timetakesuv_hrtime()under a mutex before the wait;uv__metrics_update_idle_timetakes it again after wake atlinux.c#L1564and#L1580and adds the delta toprovider_idle_time). - The reader crediting an in-flight wait, which is what
idle_entry_nsdoes here:src/uv-common.c#L1040-L1053,if (entry_time > 0) idle_time += uv_hrtime() - entry_time;
Node turns this on unconditionally for every loop it creates:
- main loop:
src/node.cc#L1587uv_loop_configure(uv_default_loop(), UV_METRICS_IDLE_TIME); - every worker loop:
src/node_worker.cc#L175uv_loop_configure(&loop_, UV_METRICS_IDLE_TIME);
and eventLoopUtilization() is computed from it in lib/internal/perf/event_loop_utilization.js via nodeTiming.idleTime, which is uv_metrics_idle_time().
One difference: libuv takes a mutex around both the entry and exit bookkeeping on every wait. This PR uses lock-free atomics for the same two stores, so the per-wait cost here is strictly lower than what every Node process (and Bun on Windows, where we run libuv) already pays. On Windows this PR does not add any measurement of its own, it just flips the same UV_METRICS_IDLE_TIME flag and reads uv_metrics_idle_time().
3ba6be2 to
466e70d
Compare
|
CI on the rebased commit (build 72296): the only red lane is debian x64-asan, where test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js died with SIGABRT. That is the known terminate() abort race on main (see #33418 and #33966, which track terminate() interrupting in-flight VM work); the test never calls eventLoopUtilization(), and 130 local debug/ASAN runs of it on this branch pass. Everything this PR touches is green, including both ELU test files. |
|
Issue #34068 was just filed reporting the same eventLoopUtilization() stub (it silently breaks @fastify/under-pressure load shedding), so this PR would fix that one too. I independently implemented the same approach on |
466e70d to
52ea52b
Compare
|
Build 72925 on the rebased sha has two red lanes, both tracked main-side and unrelated to this diff:
Every lane that exercises this diff is green, including both ELU test files. |
52ea52b to
7fa9cb8
Compare
…the clock helper - Worker::eventLoopUtilization reports zeros until 'online' (Node's kIsOnline) via !isOnline(), covering Pending as well as Closing/Closed. - The raw accessor is now eventLoopUtilizationInternal with DontEnum, matching the adjacent *Internal plumbing entries on JSWorkerPrototype. - us_internal_monotonic_ns was byte-identical to the POSIX branch of us_loop_monotonic_ns; the latter replaces it at all call sites.
dispatchOnline stores m_state from the worker thread, so sampling in the construction tick raced a fast worker boot. The entry script now blocks on a SharedArrayBuffer gate until the parent has sampled; online cannot fire before the entry finishes evaluating.
Node's gate is !kIsOnline || !kHandle; neither changes at terminate(), so values stay real until exit handling. Drop the terminate-requested term; !isOnline() still zeros Pending/Closing/Closed and the Rust side reads under vm_lock. Sample the window in the busy-worker test, and give the remaining spawn-heavy SHARE_ENV tests the same debug/ASAN ceilings.
A cross-thread prior sample taken mid-wait can exceed a later read that lands between the writer's entry-clear and idle-credit stores; without a clamp the diff surfaces as negative idle and utilization above 1. Also hardens the caller-supplied two-snapshot path.
The pre-online sample now waits for the worker to signal from inside its entry (VM published, not yet online), so the Rust-side null check cannot supply the zeros. The terminate-window sample parks the worker in a message handler on a SAB gate so its teardown cannot race the read. Verified both fail against the previous m_state >= Closing gate.
|
Build 73668: the only red lane is debian x64-asan, where test/js/web/timers/timer-heap-race.test.ts failed because LeakSanitizer caught a leaked TimeoutObject in the Atomics.waitAsync cancellation fixture (child exited SIGABRT). That allocation path (timer/mod.rs set_timeout) is untouched by this diff, the test passes 10/10 locally on this branch under ASAN, and the leak is already tracked main-side. Everything this PR touches remains green. |
2 participants
Fixes the
eventLoopUtilization()part of #32609.Repro
Before this change both return a zeroed stub and the worker variant also logs:
Cause
performance.eventLoopUtilization()(src/js/node/perf_hooks.ts) andWorker.performance.eventLoopUtilization()(src/js/node/worker_threads.ts)were hardcoded stubs returning
{ idle: 0, active: 0, utilization: 0 }. Bunnever tracked how long its event loop spent idle.
Fix
usockets now records the time the loop is blocked in the event provider. On
POSIX,
us_loop_run_bun_tickwraps theepoll_pwait2/kevent64call with amonotonic clock and accumulates the delta into a per-loop counter (only for a
real idle wait, not a zero-timeout poll-through). On Windows the idle time comes
from libuv's
uv_metrics_idle_time. A newus_loop_event_loop_utilizationreturns
idleandactive(elapsed since loop creation minus idle) innanoseconds.
performance.eventLoopUtilization()reads the current thread's loop;worker.performance.eventLoopUtilization()reads the worker's loop from theparent thread under the worker's
vm_lock(the loop lives in the worker'sarena; the lock serialises against teardown, the idle counter is read
atomically, and the creation timestamp is immutable, so no reference into the
worker VM is formed). The utilization ratio and the diff against the optional
prior-sample arguments are computed in a shared JS helper that mirrors Node's
internal/perf/event_loop_utilization.The "stub prints forever" symptom in #32609 is a separate bug:
parentPort.unref()is a no-op, so a worker with a message listener never exits. That is fixed by #30549
(
parentPort.ref()/unref()/hasRef()/close()); this PR does not touch it.Verification
bun bd test test/js/node/perf_hooks/perf_hooks.test.tsbun bd test test/js/node/worker_threads/worker_threads.test.ts -t eventLoopUtilizationNew tests assert active time after busy work, idle time after awaiting, and the
worker's activity sampled from the parent. They fail on the released binary
(return 0) and pass with the fix.
[review] gate passed · iteration 26 · 19 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 5 rejected · iteration 26
evidence per changed file
root cause · written by the author bot
The previous implementation of
Worker.performance.eventLoopUtilization()innode:worker_threadswas an unimplemented stub that emitted a NotImplementedError warning and returned zeroed values indefinitely, since Bun had no plumbing to sample event loop idle and active time from the underlying uSockets loop. The fix implements real measurement end to end: the uSockets C layer records the loop creation timestamp and atomically accumulates time spent blocked in the poll syscall, Rust FFI bindings expose those counters as idle and active durations, and a shared JS helper computes the idle, …Rebase notes (2026-07-13)
Rebased onto current main as a single squashed commit; the conflicts were non-trivial:
$newZigFunction("event_loop.zig", ...)mechanism to$newRustFunction("event_loop.rs", "jsEventLoopUtilization", 0). Main's js2native registry already mapsevent_loop.rs; thedispatch_js2native.rsre-export merged cleanly.performancegetter after node:worker_threads: +48 Node.js tests passing — MessagePort, stdio, SHARE_ENV, exit codes, transfer semantics, postMessageToThread + inspector #31216 rewroteworker_threads.tsand its test file; the getter replaces thewarnNotImplementedOncestub that PR kept, andinternal/shared.tsdrops the now-unused helper.JSWorker.cpp/internal.h/loop.cconflicts were adjacent-addition merges (new prototype entries and declarations on both sides).environmentData/getHeapSnapshot/SHARE_ENVgroups) time out at the default 5s on a loaded debug/ASAN machine, on pristine main as well; they pass with more headroom, so they got the same 60s ceilings as theexecArgvgroup. Full file is 93/93 locally on this branch.mi_on_thread_idle_end()since it only does clock reads and atomics. Also gave the new worker-name GC stress test from Fix four crash/correctness bugs: node:vm link(), Worker name, FFI threadsafe callbacks, sliced Bun.file #34140 the same 60s debug/ASAN ceiling; full file is 94/94 locally.