Skip to content

feat(ext/node): emit perf_hooks PerformanceEntry for http2 sessions and streams - #33618

Merged
littledivy merged 14 commits into
denoland:mainfrom
divybot:claude/test-http2-perf_hooks
May 3, 2026
Merged

littledivy merged 14 commits into
denoland:mainfrom
divybot:claude/test-http2-perf_hooks

Conversation

@divybot

@divybot divybot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Enables test-http2-perf_hooks in node_compat suite.

Test plan

  • cargo test --test node_compat -- test-http2-perf_hooks

divybot and others added 3 commits April 28, 2026 08:18
…nd streams

Enables tests/node_compat/runner/suite/test/parallel/test-http2-perf_hooks.js

Co-authored-by: Divy Srivastava <[email protected]>

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is well-structured. Three coordinated pieces:

  1. Rust frame counters: frames_received / frames_sent incremented in the nghttp2 on_frame_recv_callback / on_frame_send_callback, exposed via #[fast] #[smi] fn frames_received/sent() ops. Counter type is u32 with saturating_add so it won't panic on overflow at the cost of saturating after 4B frames — fine for any reasonable session.
  2. JS perf-entry plumbing: kPerfStats / kPerfEmitted Symbols on each session/stream, emitSessionPerfEntry / emitStreamPerfEntry called from finishSessionClose and the stream-close path. Uses enqueuePerformanceEntry({ name, entryType: "http2", startTime, duration, detail }) matching Node's PerformanceEntry shape for http2.
  3. Test enrollment alphabetically positioned correctly (padding-aligned < perf_hooks < ping-settings-heapdump).

Holding off on autonomous APPROVE for two reasons per fibibot's policy:

  1. CI not yet complete — 80/n checks passing, none failed, but the relevant test node_compat debug shards on most platforms are still queued.

  2. Cross-cutting infrastructure change: this PR adds two new public exports to ext/web/15_performance.js (enqueuePerformanceEntry, registerExtraEntryType) plus a module-level extraEntryTypes Set that the PerformanceObserver constructor now consults. That's not just node:http2 plumbing — it's new infra that any future polyfill (node:dns, node:http, node:net, etc.) can hook into to surface their own performance entries. Worth a maintainer's deliberate sign-off on the API shape (registerExtraEntryType global registry vs. per-observer subscription, enqueuePerformanceEntry exposing queuePerformanceEntry directly with no validation, etc.).

cc @bartlomieju on the ext/web/15_performance.js API additions — would appreciate your read on whether the extraEntryTypes registry is the right shape for general perf_hooks integration, or if the http2 case should be more contained.

Two non-blocking inline notes below.

Comment thread ext/web/15_performance.js Outdated
// about "mark" and "measure"; this lets non-web entry types be observed
// without having to fork the whole observer machinery.
const extraEntryTypes = new SafeSet();
function registerExtraEntryType(type) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API-shape note worth bartek's input on:

  • registerExtraEntryType(type) is write-once, never-deregister — module-level Set that grows monotonically. Multiple register calls for the same type are idempotent (Set handles dedup), so that's fine.
  • But the registry is global to the isolate, which means a module that registers "http2" then gets unloaded would still leave the type recognized. Probably not an issue (modules don't unload in practice), but registerExtraEntryType being global rather than scoped to the registering module makes it harder to reason about ownership.
  • No validation on type: passing a Symbol or empty string would silently corrupt the Set. Worth a typeof type !== "string" guard at minimum, since this is a public-facing helper that Node polyfills will call.

Filing these as observations on the API shape, not as blockers.

Comment thread ext/web/15_performance.js Outdated
// plain object (or PerformanceEntry) with `name`, `entryType`, `startTime`,
// `duration` properties, and any extra fields (`detail`, etc.) the consumer
// expects.
function enqueuePerformanceEntry(entry) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enqueuePerformanceEntry is currently a thin re-export of queuePerformanceEntry, which doesn't validate the entry has the expected name/entryType/startTime/duration properties. The http2 caller in this PR builds entries by hand (plain object literal, not new PerformanceEntry), and queuePerformanceEntry just passes those through to observers' entryLists.

If a future polyfill calls this with a malformed entry, observers will get garbage with no fast-fail path. Worth either:

  • Adding a webidl.converters["PerformanceEntry"]-style coercion here, so the helper produces a real PerformanceEntry (or rejects), or
  • Documenting the contract ("caller must pass a frozen plain object with these fields") and leaving validation to the call sites.

The http2 caller's entries match the contract, so this is forward-looking guidance more than a blocker.

…rformanceEntry

Reject non-string `type` and malformed entries with TypeError so polyfills fail fast instead of silently corrupting observer state.

Co-authored-by: Divy Srivastava <[email protected]>

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta addresses my prior "no validation" concern on enqueuePerformanceEntry. Verified:

  • registerExtraEntryType(type) now throws TypeError on non-string or empty-string. ✓
  • enqueuePerformanceEntry(entry) validates entry is non-null object with required fields (name, entryType, startTime, duration) of the right type. Extra fields like detail pass through untouched, which matches the docstring. ✓
  • TypeError is the primordial (imported at 15_performance.js:19), so the throw can't be hijacked. ✓
  • Validation happens before queuePerformanceEntry, so an observer never sees a malformed entry. ✓

CI fully green: 133 SUCCESS, 0 FAILURE, 2 SKIPPED. All test node_compat × all platforms × debug+release pass.

Holding to COMMENT rather than APPROVE — same architectural escalation as my prior review. The validation patches the obvious foot-gun, but the bigger call still lives with the maintainer:

  • extraEntryTypes is a global set that mutates PerformanceObserver's allowed entry types for the whole isolate. Once node:http2 calls registerExtraEntryType("http2"), every PerformanceObserver({ entryTypes: ["http2"] }) becomes legal everywhere, including in code that never touched http2.
  • An alternative (per-polyfill or per-observer subscription) would scope this. Not necessarily wrong as is, but it's a one-way door for the API shape.

cc @bartlomieju — registry-shape decision is the call worth deliberately signing off.

divybot and others added 7 commits April 28, 2026 19:37
… addressed the actionable

Enables tests/node_compat/runner/suite/test/parallel/test-http2-perf_hooks.js

Co-authored-by: Divy Srivastava <[email protected]>
Adapt enqueuePerformanceEntry/registerExtraEntryType to lazy-loaded
script pattern from denoland#33760: 15_performance.js now returns its exports
from an IIFE; consumers (perf_hooks.js, http2.ts) load via
core.loadExtScript instead of static ESM import.

Co-authored-by: Divy Srivastava <[email protected]>
…r review, normal)

Enables tests/node_compat/runner/suite/test/parallel/test-http2-perf_hooks.js

Co-authored-by: Divy Srivastava <[email protected]>
Re-added in 7d76a6f; broke top-level-entry lint check.

Co-authored-by: Divy Srivastava <[email protected]>
@divybot

divybot commented May 2, 2026

Copy link
Copy Markdown
Contributor Author

@littledivy heads-up: the bot's analysis of this PR's CI failures says they look unrelated/flaky and not addressable from this PR's diff. Verdict:

removed accidentally re-added .claude/scheduled_tasks.lock (broke top-level lint); spam comments already auto-minimized; fibibot reviews defer to maintainer

Please verify and either rerun the failing checks (admin needed), waive them, or merge if the green checks are sufficient. Pinged once; the bot won't re-engage on the same signals.

@littledivy
littledivy enabled auto-merge (squash) May 2, 2026 19:12
Resolves overlap with denoland#33796 (Http2Session PerformanceObserver entries):
- Drop changes to ext/web/15_performance.js (registerExtraEntryType /
  enqueuePerformanceEntry); main's denoland#33796 implements perf entries via a
  contained nodeObservers registry in node:perf_hooks instead, addressing
  fibibot's architectural concern.
- Use enqueueNodePerformanceEntry from node:perf_hooks; keep this PR's
  fuller emit helpers (Http2Stream entries + bytesRead/bytesWritten/
  pingRTT/streamAverageDuration/streamCount + framesSent).
- Deduplicate Session.frames_received field/initializer/op (added by both
  PRs).

Co-authored-by: Divy Srivastava <[email protected]>
auto-merge was automatically disabled May 3, 2026 04:23

Head branch was pushed to by a user without write access

@littledivy
littledivy enabled auto-merge (squash) May 3, 2026 04:25
Resolves conflict in ext/node/ops/http2/session.rs on_frame_send_callback:
combined frames_sent counter (this PR) with GOAWAY error_code capture (main denoland#33795)
in a single callback body.

Co-authored-by: Divy Srivastava <[email protected]>
auto-merge was automatically disabled May 3, 2026 06:17

Head branch was pushed to by a user without write access

@littledivy
littledivy merged commit 8617871 into denoland:main May 3, 2026
136 checks passed
littledivy added a commit to crowlKats/deno that referenced this pull request Jun 10, 2026
…d streams (denoland#33618)

Enables `test-http2-perf_hooks` in node_compat suite.

Co-authored-by: Divy Srivastava <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants