fix(ext/node): improve perf_hooks timerify and add missing exports#33581
fix(ext/node): improve perf_hooks timerify and add missing exports#33581bartlomieju merged 4 commits intomainfrom
Conversation
Rewrites timerify to match Node.js behavior: - Support constructor calls via new.target - Preserve function .name (prefixed with 'timerified ') and .length - Validate arguments with proper ERR_INVALID_ARG_TYPE errors - Accept optional histogram option with validation Also exports timerify and eventLoopUtilization as named exports from the perf_hooks module, matching Node.js API surface. Fixes 4 node_compat tests: - test-perf-hooks-timerify-return-value - test-perf-hooks-timerify-multiple-wrapping - test-perf-hooks-timerify-invalid-args - test-perf-hooks-timerify-error
lunadogbot
left a comment
There was a problem hiding this comment.
Verified the four enabled tests pass against this diff: timerified.name === 'timerified ' + fn.name (chained as timerified timerified m), length preservation, new.target routing for class constructors, and ERR_INVALID_ARG_TYPE for non-function fn and non-record histogram. Two questions inline -- one about the silent loss of the measure entry that the previous impl emitted, one about a potential TypeError: Cannot read properties of null when options is non-undefined-but-null. Neither blocks the upstream test fix.
|
Thanks for the review! Re: null options -- Good catch. Added a guard so Re: loss of measure entry -- The old |
fibibot
left a comment
There was a problem hiding this comment.
The API-shape improvements all match Node's lib/internal/perf/timerify.js:
validateFunction(fn, 'fn')→ equivalent to the newERR_INVALID_ARG_TYPEthrow ✓validateObject(options, 'options')→ equivalent ✓options.histogramvalidation with theRecordableHistogramarg-type message ✓new.targetconstructor handling (Node usesReflectConstruct(fn, args, fn), you usenew fn(...args)— same observable behavior) ✓name = 'timerified ${fn.name}'andlength = fn.length✓- The four target tests are about exactly this API shape, so they'll pass.
But there's a real semantic regression vs the old code that's worth calling out — see inline.
CI is red on dprint formatting (the long if (options !== undefined ...) line wants to wrap onto multiple lines). ./tools/format.js will fix it; full failure at https://github.com/denoland/deno/actions/runs/25001671896/job/73213243736.
| const end = performance.now(); | ||
|
|
||
| performance.measure(`timerify(${fn.name || "anonymous"})`, { start, end }); | ||
| if (options !== undefined && (typeof options !== "object" || options === null)) { |
There was a problem hiding this comment.
CI lint fails here — dprint wants this if condition wrapped:
if (
options !== undefined && (typeof options !== "object" || options === null)
) {
throw new ERR_INVALID_ARG_TYPE("options", "Object", options);
}./tools/format.js will auto-fix. Full output at https://github.com/denoland/deno/actions/runs/25001671896/job/73213243736 (3 platforms all show the same one-file diff).
|
Fixed the formatting -- pushed in 319d669. Re: the behavioral regression -- agreed this is a defensible tradeoff. The old I'd prefer option 1 (lean into Node fidelity) as a follow-up PR rather than complicating this one -- proper |
Summary
performance.timerifyto match Node.js behavior: constructorsupport via
new.target, correct.name('timerified <name>') and.lengthpreservation, properERR_INVALID_ARG_TYPEvalidation, andoptional
histogramoptiontimerifyandeventLoopUtilizationas named exports from theperf_hooksmodule, matching the Node.js API surfaceFixes 4 node_compat tests (14 -> 10 failing in the
perf_hookssuite):test-perf-hooks-timerify-return-valuetest-perf-hooks-timerify-multiple-wrappingtest-perf-hooks-timerify-invalid-argstest-perf-hooks-timerify-errorTest plan
./x test-compat parallel::test-perf-hooks-timerify-return-valuepasses./x test-compat parallel::test-perf-hooks-timerify-multiple-wrappingpasses./x test-compat parallel::test-perf-hooks-timerify-invalid-argspasses./x test-compat parallel::test-perf-hooks-timerify-errorpassesparallel::test-perf-hookssuite: 10/14 failing (was 14/14)