Conversation
Fixes oven-sh#9271 Node.js perf_hooks has performance.timerify() for measuring function execution time. Bun was missing this method. Added timerify implementation that: - Wraps functions to measure execution time - Supports both sync and async functions - Creates performance marks for measurement Before: import { performance } from 'node:perf_hooks'; const timerifyFn = performance.timerify(fn); // TypeError: performance.timerify is not a function After: performance.timerify(fn) // ✓ Works! // Creates performance measurements for function calls
WalkthroughImplements the Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/perf_hooks.ts`:
- Line 149: Replace the direct call to the user-supplied function using
fn.apply(this, args) with the safer builtin method access via fn.$apply(this,
args) to avoid relying on a potentially overridden Function.prototype.apply;
update the usage in the code around the variable fn (in perf_hooks.ts) and if
TypeScript complains, cast fn to any when accessing $apply so the call remains
equivalent but immune to user tampering.
- Line 142: Update the TypeScript declaration for the timerify function to
include an explicit this parameter so it can be bound directly from C++; locate
the timerify(fn) declaration and change its signature to include a this
parameter (for example: timerify(this: any, fn: Function) or a more specific
type if available) and ensure the return type remains unchanged; keep the
identifier timerify and adjust any corresponding TS declaration usage to accept
the new this parameter.
- Line 154: The performance.measure calls pass an incorrect value for the
duration option (they use start + duration which is actually the end timestamp);
change each call that looks like performance.measure(fn.name || 'anonymous', {
start, duration: start + duration }) to pass the true elapsed time by using the
previously computed duration variable (i.e., { start, duration }) or
alternatively supply an explicit end with { start, end: start + duration } so
that performance.measure receives either the elapsed duration or the correct end
time; update all occurrences around the performance.measure calls (the ones
using fn.name || 'anonymous') including the other two instances mentioned.
- Around line 143-145: Replace the plain typeof check and TypeError with the JSC
intrinsic and Node error helper: use $isCallable(fn) instead of typeof fn !==
'function' and throw $ERR_INVALID_ARG_TYPE('fn', 'function', fn) when the check
fails; update the validation near the existing fn argument check in perf_hooks
(the block that currently throws new TypeError('The "fn" argument must be of
type function')) so it uses $isCallable and $ERR_INVALID_ARG_TYPE for
consistency with other validators.
- Around line 151-156: The Promise branch only measures on fulfillment; add
handling for rejections so we record a measurement when the returned promise
rejects. Update the result.then(...) call in the async branch to either supply a
rejection handler as the second argument or chain .catch(err => { const duration
= performance.now() - start; performance.measure(fn.name || 'anonymous', {
start, duration: start + duration }); throw err; }) so the measurement (using
performance.measure with start and computed duration) is recorded on both
resolve and reject while rethrowing the original error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9262e98-fb1b-4612-a441-fd91ec423246
📒 Files selected for processing (1)
src/js/node/perf_hooks.ts
| setResourceTimingBufferSize(_) { | ||
| return performance.setResourceTimingBufferSize(...arguments); | ||
| }, | ||
| timerify(fn) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add this parameter typing for TypeScript.
Per coding guidelines, builtin functions must include this parameter typing in TypeScript to enable direct method binding in C++.
♻️ Proposed fix
- timerify(fn) {
+ timerify(this: void, fn: (...args: unknown[]) => unknown) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| timerify(fn) { | |
| timerify(this: void, fn: (...args: unknown[]) => unknown) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/perf_hooks.ts` at line 142, Update the TypeScript declaration for
the timerify function to include an explicit this parameter so it can be bound
directly from C++; locate the timerify(fn) declaration and change its signature
to include a this parameter (for example: timerify(this: any, fn: Function) or a
more specific type if available) and ensure the return type remains unchanged;
keep the identifier timerify and adjust any corresponding TS declaration usage
to accept the new this parameter.
| if (typeof fn !== 'function') { | ||
| throw new TypeError('The "fn" argument must be of type function'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use $isCallable() and $ERR_INVALID_ARG_TYPE for consistency with other validators.
Per coding guidelines, use JSC intrinsics like $isCallable() for type checking and throw $ERR_* error codes for invalid arguments to match Node.js error format.
♻️ Proposed fix
timerify(fn) {
- if (typeof fn !== 'function') {
- throw new TypeError('The "fn" argument must be of type function');
+ if (!$isCallable(fn)) {
+ throw $ERR_INVALID_ARG_TYPE('fn', 'function', fn);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/perf_hooks.ts` around lines 143 - 145, Replace the plain typeof
check and TypeError with the JSC intrinsic and Node error helper: use
$isCallable(fn) instead of typeof fn !== 'function' and throw
$ERR_INVALID_ARG_TYPE('fn', 'function', fn) when the check fails; update the
validation near the existing fn argument check in perf_hooks (the block that
currently throws new TypeError('The "fn" argument must be of type function')) so
it uses $isCallable and $ERR_INVALID_ARG_TYPE for consistency with other
validators.
| return function (...args) { | ||
| const start = performance.now(); | ||
| try { | ||
| const result = fn.apply(this, args); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use .$apply() instead of .apply() to prevent user tampering.
As per coding guidelines, builtin modules should use .$apply() instead of .apply() to prevent users from overriding Function.prototype.apply.
♻️ Proposed fix
- const result = fn.apply(this, args);
+ const result = fn.$apply(this, args);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = fn.apply(this, args); | |
| const result = fn.$apply(this, args); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/perf_hooks.ts` at line 149, Replace the direct call to the
user-supplied function using fn.apply(this, args) with the safer builtin method
access via fn.$apply(this, args) to avoid relying on a potentially overridden
Function.prototype.apply; update the usage in the code around the variable fn
(in perf_hooks.ts) and if TypeScript complains, cast fn to any when accessing
$apply so the call remains equivalent but immune to user tampering.
| if (result && typeof result.then === 'function') { | ||
| return result.then((value) => { | ||
| const duration = performance.now() - start; | ||
| performance.measure(fn.name || 'anonymous', { start, duration: start + duration }); | ||
| return value; | ||
| }); |
There was a problem hiding this comment.
Promise rejection not measured.
When the wrapped function returns a promise that rejects, no measurement is recorded. The sync catch block only handles synchronous throws, not async rejections. Consider adding a .catch() or second argument to .then() to record measurements on rejection.
🛡️ Proposed fix to handle promise rejection
if (result && typeof result.then === 'function') {
- return result.then((value) => {
- const duration = performance.now() - start;
- performance.measure(fn.name || 'anonymous', { start, duration });
- return value;
- });
+ return result.then(
+ (value) => {
+ const duration = performance.now() - start;
+ performance.measure(fn.name || 'anonymous', { start, duration });
+ return value;
+ },
+ (error) => {
+ const duration = performance.now() - start;
+ performance.measure(fn.name || 'anonymous', { start, duration });
+ throw error;
+ }
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/perf_hooks.ts` around lines 151 - 156, The Promise branch only
measures on fulfillment; add handling for rejections so we record a measurement
when the returned promise rejects. Update the result.then(...) call in the async
branch to either supply a rejection handler as the second argument or chain
.catch(err => { const duration = performance.now() - start;
performance.measure(fn.name || 'anonymous', { start, duration: start + duration
}); throw err; }) so the measurement (using performance.measure with start and
computed duration) is recorded on both resolve and reject while rethrowing the
original error.
| if (result && typeof result.then === 'function') { | ||
| return result.then((value) => { | ||
| const duration = performance.now() - start; | ||
| performance.measure(fn.name || 'anonymous', { start, duration: start + duration }); |
There was a problem hiding this comment.
Bug: Incorrect duration value passed to performance.measure().
The duration option should be the elapsed time itself, not start + duration. Currently this computes start + (performance.now() - start) which equals performance.now() — the absolute end time, not the duration.
Use either { start, duration } or { start, end: performance.now() }:
🐛 Proposed fix
if (result && typeof result.then === 'function') {
return result.then((value) => {
const duration = performance.now() - start;
- performance.measure(fn.name || 'anonymous', { start, duration: start + duration });
+ performance.measure(fn.name || 'anonymous', { start, duration });
return value;
});
}
const duration = performance.now() - start;
- performance.measure(fn.name || 'anonymous', { start, duration: start + duration });
+ performance.measure(fn.name || 'anonymous', { start, duration });
return result;
} catch (error) {
const duration = performance.now() - start;
- performance.measure(fn.name || 'anonymous', { start, duration: start + duration });
+ performance.measure(fn.name || 'anonymous', { start, duration });
throw error;
}Also applies to: 159-159, 163-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/perf_hooks.ts` at line 154, The performance.measure calls pass an
incorrect value for the duration option (they use start + duration which is
actually the end timestamp); change each call that looks like
performance.measure(fn.name || 'anonymous', { start, duration: start + duration
}) to pass the true elapsed time by using the previously computed duration
variable (i.e., { start, duration }) or alternatively supply an explicit end
with { start, end: start + duration } so that performance.measure receives
either the elapsed duration or the correct end time; update all occurrences
around the performance.measure calls (the ones using fn.name || 'anonymous')
including the other two instances mentioned.
Fixes #9271
Node.js
perf_hooksmodule providesperformance.timerify()to measure function execution time. Bun's implementation was missing this method.Problem
Solution
Added
performance.timerify()method that:performance.measure()Implementation
Usage
Changes
src/js/node/perf_hooks.tstimerifymethod to performance object