Skip to content

fix: add performance.timerify method to perf_hooks #27887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ssing2 wants to merge 1 commit into from
Closed

fix: add performance.timerify method to perf_hooks #27887

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/js/node/perf_hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,32 @@ export default {
setResourceTimingBufferSize(_) {
return performance.setResourceTimingBufferSize(...arguments);
},
timerify(fn) {

@coderabbitai coderabbitai Bot Mar 7, 2026

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.

🧹 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.

Suggested change
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');
}
Comment on lines +143 to +145

@coderabbitai coderabbitai Bot Mar 7, 2026

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.

🧹 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);

@coderabbitai coderabbitai Bot Mar 7, 2026

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.

🛠️ 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.

Suggested change
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 is a promise, measure when it resolves
if (result && typeof result.then === 'function') {
return result.then((value) => {
const duration = performance.now() - start;
performance.measure(fn.name || 'anonymous', { start, duration: start + duration });

@coderabbitai coderabbitai Bot Mar 7, 2026

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.

⚠️ Potential issue | 🔴 Critical

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.

return value;
});
Comment on lines +151 to +156

@coderabbitai coderabbitai Bot Mar 7, 2026

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.

⚠️ Potential issue | 🟡 Minor

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.

}
const duration = performance.now() - start;
performance.measure(fn.name || 'anonymous', { start, duration: start + duration });
return result;
} catch (error) {
const duration = performance.now() - start;
performance.measure(fn.name || 'anonymous', { start, duration: start + duration });
throw error;
}
};
},
timeOrigin: performance.timeOrigin,
toJSON(_) {
return performance.toJSON(...arguments);
Expand Down