Skip to content

node:perf_hooks: split Histogram into base/Recordable/ELD classes - #33587

Closed
robobun wants to merge 3 commits into
mainfrom
farm/c2814560/histogram-class-hierarchy
Closed

robobun wants to merge 3 commits into
mainfrom
farm/c2814560/histogram-class-hierarchy

Conversation

@robobun

@robobun robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Repro

import { createHistogram, monitorEventLoopDelay } from "node:perf_hooks";

const eld = monitorEventLoopDelay();
typeof eld.record;        // node: "undefined"   bun: "function"
eld.record(123456789);    // node: TypeError     bun: injects a fake 123ms loop-delay sample

const h = createHistogram(); h.record(5);
h.add(eld);               // node: TypeError ERR_INVALID_ARG_TYPE   bun: accepted, count=2
h.add(createHistogram()); // node: undefined     bun: 0

Bun 1.4.0 and current main put every Histogram method on one prototype, so the monitorEventLoopDelay() result exposes record/recordDelta/add and RecordableHistogram.prototype.add accepts it as an argument. Node's hierarchy keeps the event-loop-delay histogram read-only so its samples can only come from the timer.

Cause

JSNodePerformanceHooksHistogram had a single prototype carrying both the read-only stat surface and the mutation methods, and jsFunction_monitorEventLoopDelay allocated with the same structure as createHistogram. add() accepted any native histogram instance and returned the dropped-sample count.

Fix

  • Split the native prototype: JSNodePerformanceHooksHistogramPrototype keeps the read-only getters plus reset/percentile/percentileBigInt; a new JSNodePerformanceHooksRecordableHistogramPrototype (chained to the base) owns record/recordDelta/add. The Symbol.toStringTag is dropped so both variants stringify as [object Object].
  • Add HistogramKind { Recordable, Interval } to the instance. record/recordDelta/add reject a non-recordable receiver with ERR_INVALID_THIS; add() rejects a non-recordable argument with ERR_INVALID_ARG_TYPE("other", "RecordableHistogram", …) and returns undefined.
  • Add m_JSNodePerformanceHooksIntervalHistogramStructure so jsFunction_monitorEventLoopDelay allocates with a structure whose prototype is the read-only base.
  • monitorEventLoopDelay.ts builds an ELDHistogram prototype (enable/disable/Symbol.dispose) on top of the base and installs it on the native instance, giving eld.constructor.name === "ELDHistogram" and the ELDHistogram -> Histogram -> Object chain.
  • Rename the constructor to RecordableHistogram so createHistogram().constructor.name matches Node.

Verification

bun bd test test/js/bun/perf_hooks/histogram.test.ts                  # 45 pass
bun bd test/js/node/test/sequential/test-performance-eventloopdelay.js # exit 0

The new class hierarchy describe block fails on released Bun 1.4.0:

(fail) Histogram > class hierarchy > monitorEventLoopDelay() histogram does not expose record/recordDelta/add
(fail) Histogram > class hierarchy > add() rejects an event-loop-delay histogram
(fail) Histogram > class hierarchy > add() returns undefined
(fail) Histogram > class hierarchy > record()/add() borrowed onto an event-loop-delay histogram throws ERR_INVALID_THIS
(fail) Histogram > class hierarchy > prototype chain

monitorEventLoopDelay() returned the same flat RecordableHistogram as
createHistogram(), so user code could inject fake samples into the
event-loop-delay histogram via .record() and RecordableHistogram.add()
accepted an ELD histogram where Node throws ERR_INVALID_ARG_TYPE.

The native prototype is now split into a read-only Histogram base and a
RecordableHistogram subclass that alone carries record/recordDelta/add.
monitorEventLoopDelay() instances use a separate structure rooted at the
base and get an ELDHistogram prototype (enable/disable/Symbol.dispose)
layered on in JS. A HistogramKind tag on each instance lets
record/recordDelta/add reject a non-recordable receiver with
ERR_INVALID_THIS and add() reject a non-recordable argument with
ERR_INVALID_ARG_TYPE. add() now returns undefined and the toStringTag is
dropped so both kinds stringify as [object Object].
@github-actions github-actions Bot added the claude label Jul 7, 2026
@coderabbitai

coderabbitai Bot commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a33d3903-3ede-4a08-86e0-0792aa022326

📥 Commits

Reviewing files that changed from the base of the PR and between a780f57 and a86effe.

📒 Files selected for processing (9)
  • src/js/internal/perf_hooks/monitorEventLoopDelay.ts
  • src/jsc/bindings/JSNodePerformanceHooksHistogram.cpp
  • src/jsc/bindings/JSNodePerformanceHooksHistogram.h
  • src/jsc/bindings/JSNodePerformanceHooksHistogramConstructor.cpp
  • src/jsc/bindings/JSNodePerformanceHooksHistogramPrototype.cpp
  • src/jsc/bindings/JSNodePerformanceHooksHistogramPrototype.h
  • src/jsc/bindings/ZigGlobalObject.cpp
  • src/jsc/bindings/ZigGlobalObject.h
  • test/js/bun/perf_hooks/histogram.test.ts

Walkthrough

Introduces a HistogramKind enum (Recordable, Interval) that classifies native histograms, splits the JS class hierarchy into a base Histogram prototype and a derived RecordableHistogram prototype, restricts record/recordDelta/add to Recordable histograms, adds an interval histogram structure for monitorEventLoopDelay, updates the JS wrapper to cache a shared prototype, and adds corresponding tests.

Changes

Histogram kind refactor

Layer / File(s) Summary
HistogramKind enum and constructor contract
src/jsc/bindings/JSNodePerformanceHooksHistogram.h, src/jsc/bindings/JSNodePerformanceHooksHistogram.cpp
Adds HistogramKind enum, threads a kind parameter through the constructor, create factories, and adds a kind() accessor and m_kind member.
Constructor and prototype class hierarchy split
src/jsc/bindings/JSNodePerformanceHooksHistogramConstructor.cpp, src/jsc/bindings/JSNodePerformanceHooksHistogramPrototype.h
Builds a base Histogram prototype guarded by an illegal-constructor function, derives a RecordableHistogram prototype/structure from it, and adds a new interval structure factory declaration.
Method kind validation
src/jsc/bindings/JSNodePerformanceHooksHistogramPrototype.cpp
Moves record/recordDelta/add onto the recordable prototype table, validates this (and add's argument) is HistogramKind::Recordable using Bun-standardized errors, and changes add to return undefined.
Histogram creation wiring
src/jsc/bindings/JSNodePerformanceHooksHistogramPrototype.cpp, src/jsc/bindings/ZigGlobalObject.cpp, src/jsc/bindings/ZigGlobalObject.h
Updates createHistogram/monitorEventLoopDelay creation to pass the correct HistogramKind, and registers a lazily-initialized interval histogram structure as a GC member.
monitorEventLoopDelay JS prototype caching
src/js/internal/perf_hooks/monitorEventLoopDelay.ts
Types the native histogram as IntervalHistogram, adds an ELDHistogram constructor stub throwing on illegal construction, and caches a shared prototype for enable/disable/Symbol.dispose instead of defining these per instance.
Class hierarchy tests
test/js/bun/perf_hooks/histogram.test.ts
Adds a "class hierarchy" test suite validating method availability, add() error handling and return value, ERR_INVALID_THIS on borrowed methods, and prototype/constructor relationships.

Compact metadata

  • Estimated review effort: High
  • Lines changed: approximately +232/-49 across 10 files

Suggested labels: perf_hooks, jsc-bindings, needs-review

Suggested reviewers: maintainers familiar with JSC bindings and perf_hooks internals

🐰 A histogram once was one shape alone,
Now Recordable and Interval each have their throne,
With prototypes stacked and kinds checked with care,
The event loop's delay is measured with flair,
Tests confirm the hierarchy — hop, all is known!

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: splitting perf_hooks histograms into base, Recordable, and ELD classes.
Description check ✅ Passed The description is detailed and covers the change, cause, fix, and verification, which satisfies the template intent despite different headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:05 AM PT - Jul 7th, 2026

❌ @robobun, your commit ba6a780 has some failures in Build #69765 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 33587

That installs a local version of the PR into your bun-33587 executable, so you can run:

bun-33587 --bun

@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: every test lane that ran passed (alpine, debian incl. x64-asan, darwin-26-aarch64, windows, freebsd builds). The two red checks are darwin-14-aarch64-test-bun and darwin-14-x64-test-bun, both marked Expired by Buildkite (no agent picked them up, so no tests ran). Build 69545 before the retrigger had the same pattern plus an artifact-download timeout on darwin-26.

No histogram / perf_hooks failure appears in any annotation across either build. The diff is green; the remaining red is macOS-14 agent capacity.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-07, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant