perf_hooks: record nodeTiming milestones and resolve their names in performance.measure() - #38477
perf_hooks: record nodeTiming milestones and resolve their names in performance.measure()#38477robobun wants to merge 1 commit into
Conversation
…ser Timing performance.nodeTiming reported timeOrigin (an epoch timestamp) for every startup milestone, and performance.measure() could not reference the milestone names because the native User Timing code only knew user marks. The VM now stamps nodeStart, v8Start, environment, bootstrapComplete, loopStart and loopExit relative to origin_timer, exposed through Bun__getNodeTimingMilestone. PerformanceUserTiming gets back WebKit's restricted-mark-name table, pointed at those milestones: measure() resolves the six names to the milestone values, while mark(), new PerformanceMark() and clearMarks() reject them with ERR_INVALID_ARG_VALUE like Node. perf_hooks' nodeTiming reads the same values through own accessor properties, matching Node's object shape. Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit f8a07b3 has some failures in 🧪 To try this PR locally: bunx bun-pr 38477That installs a local version of the PR into your bun-38477 --bun |
|
Status: ready for review; needs a maintainer to merge (or to retry one CI lane, see below). Reproduced on bun 1.4.0 and a debug build of main with the snippet from the report: all four Proof: the new CI on f8a07b3 (build 95952): every test lane that ran passed (177 jobs, including macOS 14 x64 and macOS 26 arm64). The only thing marked failed is |
|
Warning Review limit reached
Next review available in: 15 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 (15)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It touches core VM lifecycle paths (init, auto_tick, on_before_exit, reload_entry_point) and makes judgement calls about where Bun's startup phases map to Node's milestone semantics, plus it overlaps with three open PRs — a human look at the milestone placement and PR coordination is worthwhile.
What was reviewed:
- Milestone stamp sites in
VirtualMachine.rsand the raw-pointer projection inrecord_node_timing_milestone_rawduring partial init — only touches already-written fields. SortedArrayMapkey ordering (alphabetical, correct) and theclearMarksExceptionOr<void>return flowing throughtoJS<IDLUndefined>.EnumTraits<ExceptionCode>not listing the new variant — matches pre-existing omissions (EVENT_RECURSION,OutOfMemoryError); not load-bearing here.- Per-tick cost of
record_node_timing_milestoneinauto_tick— one indexed load + branch, inlined.
Extended reasoning...
Overview
This PR implements real performance.nodeTiming milestones and Node's reserved-name behaviour in User Timing. It spans 15 files across four layers: Rust VM state (VirtualMachine.rs adds a 6-slot [i64] array stamped at startup phases and exposes it via a new Bun__getNodeTimingMilestone FFI export), C++ WebCore bindings (PerformanceUserTiming gains a SortedArrayMap of the six names, convertMarkToTimestamp resolves them, PerformanceMark::create and clearMarks reject them via a new ExceptionCode::InvalidArgValueError mapped in createDOMException), the built-in JS module (perf_hooks.ts rewrites PerformanceNodeTiming as own-enumerable getter properties backed by a $newCppFunction host call), and tests (a 230-line describe block covering shape, measure() resolution, mark()/clearMarks() rejection, loopStart/loopExit lifecycle in subprocesses, workers, and process.exit(); plus Node's vendored test-performance-nodetiming.js).
Security risks
None identified. No user-controlled data reaches size arithmetic, allocation, or filesystem/network paths. The new FFI export bounds-checks its u32 index via .get(index as usize) and returns -1.0 on miss. The C++ SortedArrayMap is a compile-time constant table.
Level of scrutiny
High. The change is not mechanical: it instruments VirtualMachine::init (which builds the VM from a partially-initialized raw pointer), the per-tick hot path auto_tick/auto_tick_active, and the on_before_exit shutdown sequence (including a unhandled_error_counter == 0 gate on LoopExit). Each stamp site is a design decision about which Bun phase corresponds to which Node milestone, and auto_tick runs on every event-loop turn. The clearMarks return-type change from void to ExceptionOr<void> flows through generated-binding-style code. These are the kinds of placement and layering choices a maintainer should confirm.
Other factors
- Test coverage is thorough and follows repo conventions (subprocess tests drain all pipes concurrently, use
test.concurrent, assert stderr/stdout before exitCode, cover the variant matrix including workers and the no-async-work path). - The PR description explicitly names three overlapping open PRs (#32481, #36069, #35390); coordinating supersession/rebase is a human decision.
- The
record_node_timing_milestone_rawunsafe helper is carefully scoped (only projects the two already-initialized fields, no&mut VirtualMachineformed) with a SAFETY contract, but touching the partial-init block ininitwarrants a second pair of eyes. - The new
DOMExceptionCode::EventRecursionvariant added on the Rust side alongsideInvalidArgValueErrorfills a pre-existing gap (C++ hadEVENT_RECURSION, Rust didn't) — harmless but worth a maintainer noting.
|
For whoever reviews the milestone placement, the mapping in one place (Node's definition on the left, where Bun stamps it on the right):
Every stamp is first-write-wins, so hot reloads and repeated ticks keep the original values. Workers run through the same On the overlapping PRs: this one only needs to land on its own; #32481 becomes unnecessary, #36069 would drop its |
2 participants
Problem
performance.measure("boot", "nodeStart", "bootstrapComplete")(and the other fivenodeTimingnames, in any of measure's argument shapes) throwsSyntaxError: No mark named 'bootstrapComplete' exists. Node documents that measure() accepts these names and resolves them toperformance.nodeTiming.performance.mark("nodeStart"),new PerformanceMark("nodeStart")andperformance.clearMarks("nodeStart")succeed; Node rejects the six names withERR_INVALID_ARG_VALUE(The argument 'name' is invalid. Received 'nodeStart').performance.nodeTimingitself was fake:nodeStart/v8Start/environment/bootstrapCompletewere allperformance.timeOrigin(an epoch timestamp, ~1.78e12),loopStartandidleTimewere1,startTimewas the epoch value, and the values were data properties frozen at module load (src/js/node/perf_hooks.tscreatePerformanceNodeTiming). Node reports milliseconds sincetimeOrigin,-1for unreached milestones, andstartTime: 0.PerformanceUserTiming::convertMarkToTimestamp(src/jsc/bindings/webcore/PerformanceUserTiming.cpp) only consulted the user marks map; the browser version's restricted-name table had been removed in the port, and nothing in the runtime recorded startup milestones for it to resolve against.Fix
VirtualMachine(src/jsc/VirtualMachine.rs) records six milestones as nanoseconds sinceorigin_timer(the same clockperformance.now()uses), first write wins:nodeStartright afterorigin_timeris taken,v8Startjust before the JSC VM and global object are created,environmentat the end ofinit,bootstrapCompleteinreload_entry_point(main entry, workers,-e) and in the test runner's equivalent,loopStartinauto_tick/auto_tick_active(pluson_before_exit, for scripts that never poll the loop),loopExitat the end ofon_before_exit.process.exit()and a fatal uncaught exception never reach that point, soloopExitstays-1for'exit'listeners, as in Node.Bun__getNodeTimingMilestone(vm, index)(src/jsc/virtual_machine_exports.rs) returns a milestone in ms or-1.PerformanceUserTiminggets WebKit's restricted-mark-name mechanism back, with the six names mapped to milestone indices:convertMarkToTimestampreturns the milestone for them (including-1, which is what Node returns for an unreachedloopStart/loopExit),PerformanceMark::create(coversmark()and the constructor) andclearMarks()reject them. The rejection is a newExceptionCode::InvalidArgValueError, mapped toERR_INVALID_ARG_VALUEincreateDOMExceptionthe same wayEVENT_RECURSIONis;clearMarksnow returnsExceptionOr<void>, which the existing binding already handles.PerformanceNodeTimingis ported from Node's nodetiming.js: own enumerable properties,startTime0, milestones as getters backed by a small host function (jsPerformance_getNodeTimingMilestone, which goes through the same name table), someasure("x", name).startTime === nodeTiming[name]holds at every point in the process lifetime.idleTimeis 0, consistent with theeventLoopUtilization()stub.nodeTiming.test/js/node/perf_hooks/perf_hooks.test.ts(newnodeTiming milestonesblock: 6 of 7 tests fail on main, all pass with the fix); Node'stest/parallel/test-performance-nodetiming.jsvendored verbatim from v26.3.0 (fails on main at its firststartTimeassertion, passes with the fix); the existing vendoredtest-performance-*/test-perf-hooks-*files,test/js/web/timers/performance*.test.*andtest/js/deno/performance/performance.test.tsstill pass. Expected values in the new tests were checked against node v26.3.0.nodeTimingvalues in JS only (this supersedes it); perf_hooks: match Node's User Timing argument-validation contract #36069 adds themark()rejection as part of a wider validation change (it explicitly leaves measure() resolution out); the nodeTiming part of the node:v8 profiling APIs, real perf_hooks nodeTiming, --diagnostic-dir and --heapsnapshot-signal (+11 tests) #35390 draft uses the same milestone indices and export name as this PR, so rebasing it onto this should be a matter of dropping its copy.Background
performance.nodeTiming(node:perf_hooks) is aPerformanceEntrywhose properties are timestamps of the process's startup phases, in milliseconds relative toperformance.timeOrigin, like every other User Timing value. Node also letsperformance.measure()'s start/end arguments name one of those properties instead of a user mark, and reserves the names so a user mark cannot shadow them.performance.timeOriginis the moment the thread'sVirtualMachinewas set up (origin_timer), andperformance.now()isBun__readOriginTimer= elapsed time on that clock; the milestones are stored on the same clock so they are directly comparable with marks andnow().ExceptionOr/Exceptionis how the ported WebCore code reports errors without touching JS;createDOMException(src/jsc/bindings/JSDOMExceptionHandling.cpp) turns theExceptionCodeinto the JS error when the binding returns. A few codes there already produce Node-style errors with a.codeproperty instead of a DOMException;InvalidArgValueErroris one more of those.// HOST_EXPORT(Name, c)on a Rust function makes the build emit anextern "C"thunk of that name, which is how C++ reachesBun__getNodeTimingMilestone;$newCppFunctionin a builtin module wraps a C++ host function as a JS function, which is how perf_hooks.ts reads the values.[review] gate passed · iteration 0 · 15 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file