feat(perf_hooks): add performance.timerify() implementation - #27921
feat(perf_hooks): add performance.timerify() implementation#27921ssing2 wants to merge 2 commits into
Conversation
Fixes oven-sh#27481 Documents that WAL sidecar files (.db-wal, .db-shm) cleanup behavior varies by platform and SQLite configuration after db.close(): - Linux: sidecar files typically cleaned up after close - macOS: sidecar files may persist after close - Windows: behavior varies by version and configuration Provides guidance for manual cleanup when needed.
Fixes oven-sh#9271 Adds performance.timerify() method to wrap functions and automatically measure their execution time using Performance API. ### Changes - Implemented timerify() in node:perf_hooks module - Supports both synchronous and asynchronous functions - Automatically creates PerformanceMeasure entries with function name - Handles errors properly by still measuring execution time ### Example ```ts import { performance } from 'node:perf_hooks'; const fn = () => { console.log('works'); }; const timerifyFn = performance.timerify(fn); timerifyFn(); // Creates PerformanceMeasure entry: 'works()' ``` ### Testing The implementation ensures: 1. Sync functions are measured correctly 2. Async functions return promises and are measured after resolution 3. Functions with errors are still measured 4. Anonymous functions use 'anonymous' as measure name 5. Matches Node.js performance.timerify() behavior
WalkthroughAdds documentation about SQLite WAL sidecar files and their cleanup behavior, and implements the Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/runtime/sqlite.mdx`:
- Around line 145-151: Update the cleanup example so rmSync calls won’t throw
when sidecar files are absent: after calling db.close(), use a non-throwing
removal (either rmSync with the { force: true } option or guard with
fs.existsSync) for "mydb.sqlite-wal" and "mydb.sqlite-shm" instead of plain
rmSync; keep the import of rmSync and/or add import of existsSync if you choose
the existence-check approach and ensure the example remains cross-platform and
non-throwing.
In `@src/js/node/perf_hooks.ts`:
- Line 148: Replace the direct, tamperable invocation fn.apply(this, args) with
the runtime-safe built-in invocation fn.$apply(this, args) in the wrapper that
calls the user function (the line creating const result). Locate the wrapper
where fn is invoked (the const result = ... statement) and change the call to
use .$apply so the builtin module follows the runtime safety rules for builtin
JS modules.
- Around line 151-157: The async branch only records timing on fulfillment;
change the Promise handling for the case where (result && typeof result.then ===
"function") so the end time is recorded for both fulfillment and rejection.
Replace the current result.then(value => { ... }) usage with a finalizer-style
handler (either result.finally(...) plus a then to preserve value/error, or use
result.then(value => { record end/measure; return value }, err => { record
end/measure; throw err })) so that performance.measure(`${name}()` , start, end)
is executed on both resolve and reject while preserving the original return
value or rethrowing the original error; keep referencing the same symbols:
result, name, start, performance.measure.
- Around line 155-166: The calls to performance.measure are using the wrong
overload by casting numeric timestamps to strings; replace those calls to use
the options-object overload so numeric start/end values are passed (e.g., call
performance.measure(name, { start, end })) instead of
performance.measure(`${name}()`, start as unknown as string, end as unknown as
string); update every occurrence (the async branch where value is awaited, the
sync branch returning result, and the catch block) to pass the numeric start and
end variables via the { start, end } object and remove the string casts.
- Around line 150-166: The timerify wrapper must validate the callable, use
fn.$apply(this, args), create proper start/end marks (not numeric casts) and
ensure measurements emit a "function" entry type and handle promise rejections;
specifically, in timerify: first check $isCallable(fn) before invoking, call
fn.$apply(this, args) to execute, create unique start/end mark names (e.g.,
`${name}-start-${id}` / `${name}-end-${id}`) and call performance.mark for each,
then call performance.measure(name, startMark, endMark) (not passing numeric
timestamps), and for async results attach both .then and .catch to record the
end mark/measure in both fulfillment and rejection paths; if the environment
supports constructing custom PerformanceEntry of type "function", prefer
emitting that entry type for PerformanceObserver.observe({ entryTypes:
["function"] }), otherwise fall back to creating a "measure" entry, and ensure
all marks/measures are cleared afterward.
- Around line 142-143: Validate the timerify argument immediately: in
timerify(fn: ...) call the appropriate validator from internal/validators (e.g.,
validators.validateFunction(fn, 'fn') or validators.checkFunction) at the start
of the method and throw the corresponding Node-style ERR_* (e.g.,
ERR_INVALID_ARG_TYPE) for non-callables instead of deferring validation to the
returned wrapper; ensure you import the validator and perform the check before
computing name or returning the wrapper so non-callable inputs error
synchronously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7157b82d-708c-49ce-822a-2560b9140ec4
📒 Files selected for processing (2)
docs/runtime/sqlite.mdxsrc/js/node/perf_hooks.ts
| If you need to ensure sidecar files are cleaned up, consider manually removing them after closing the database: | ||
| ```ts | ||
| import { rmSync } from "node:fs"; | ||
|
|
||
| db.close(); | ||
| rmSync("mydb.sqlite-wal"); | ||
| rmSync("mydb.sqlite-shm"); |
There was a problem hiding this comment.
Make the cleanup example non-throwing.
This snippet will throw ENOENT when one of the sidecar files was already removed or never created, which is one of the cases this note is describing. Use force: true or an existence check so the example works cross-platform.
Proposed fix
- rmSync("mydb.sqlite-wal");
- rmSync("mydb.sqlite-shm");
+ rmSync("mydb.sqlite-wal", { force: true });
+ rmSync("mydb.sqlite-shm", { force: true });📝 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.
| If you need to ensure sidecar files are cleaned up, consider manually removing them after closing the database: | |
| ```ts | |
| import { rmSync } from "node:fs"; | |
| db.close(); | |
| rmSync("mydb.sqlite-wal"); | |
| rmSync("mydb.sqlite-shm"); | |
| If you need to ensure sidecar files are cleaned up, consider manually removing them after closing the database: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/runtime/sqlite.mdx` around lines 145 - 151, Update the cleanup example
so rmSync calls won’t throw when sidecar files are absent: after calling
db.close(), use a non-throwing removal (either rmSync with the { force: true }
option or guard with fs.existsSync) for "mydb.sqlite-wal" and "mydb.sqlite-shm"
instead of plain rmSync; keep the import of rmSync and/or add import of
existsSync if you choose the existence-check approach and ensure the example
remains cross-platform and non-throwing.
| timerify(fn: (...args: unknown[]) => unknown) { | ||
| const name = fn.name || "anonymous"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What error does Node.js performance.timerify() throw when its first argument is not callable?
💡 Result:
If performance.timerify() is called with a non-callable first argument, it throws:
TypeError [ERR_INVALID_ARG_TYPE]: The "fn" argument must be of type function. Received … (with the “Received …” part describing the actual value/type passed). [1][2]
🏁 Script executed:
cd src/js/node && wc -l perf_hooks.tsRepository: oven-sh/bun
Length of output: 72
🏁 Script executed:
cat -n src/js/node/perf_hooks.ts | sed -n '140,160p'Repository: oven-sh/bun
Length of output: 1012
🏁 Script executed:
rg -A 20 "timerify\s*\(" src/js/node/perf_hooks.tsRepository: oven-sh/bun
Length of output: 917
🏁 Script executed:
rg "\$ERR_INVALID_ARG_TYPE" src/js/node/perf_hooks.ts | head -5Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
rg "\$isCallable" src/js/node/perf_hooks.ts | head -5Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
head -30 src/js/node/perf_hooks.tsRepository: oven-sh/bun
Length of output: 1019
🏁 Script executed:
rg "import.*validators|from.*validators|ERR_|isCallable" src/js/node/perf_hooks.tsRepository: oven-sh/bun
Length of output: 588
🏁 Script executed:
rg "^\s*(import|const|let)\s+(\$|ERR_|isCallable)" src/js/node/perf_hooks.ts | head -20Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
rg "^(import|const|var)" src/js/node/perf_hooks.ts | head -30Repository: oven-sh/bun
Length of output: 258
🏁 Script executed:
grep -n "require.*validators\|import.*validators" src/js/node/perf_hooks.tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
cat -n src/js/node/perf_hooks.ts | grep -A 5 -B 5 "timerify"Repository: oven-sh/bun
Length of output: 509
🏁 Script executed:
head -100 src/js/node/perf_hooks.ts | cat -nRepository: oven-sh/bun
Length of output: 3664
🏁 Script executed:
rg -B 5 -A 5 "\$ERR_" src/js/node/perf_hooks.ts | head -40Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
cat -n src/js/node/perf_hooks.ts | sed -n '1,50p'Repository: oven-sh/bun
Length of output: 2127
🏁 Script executed:
rg "ERR_INVALID_ARG_TYPE|isCallable" src/js/node/ --type tsRepository: oven-sh/bun
Length of output: 14576
🏁 Script executed:
fd "validators" src/js/ --type fRepository: oven-sh/bun
Length of output: 84
🏁 Script executed:
cat -n src/js/node/perf_hooks.ts | sed -n '130,175p'Repository: oven-sh/bun
Length of output: 2017
🏁 Script executed:
grep -n "\$ERR_INVALID_ARG_TYPE\|\$isCallable" src/js/node/perf_hooks.tsRepository: oven-sh/bun
Length of output: 328
Validate fn upfront before returning the wrapper.
timerify() must reject non-callable arguments immediately, matching Node.js behavior. Currently, validation is deferred until the wrapper is invoked, which violates the API contract and defers the error.
Proposed fix
timerify(fn: (...args: unknown[]) => unknown) {
+ if (!$isCallable(fn)) {
+ throw $ERR_INVALID_ARG_TYPE("fn", "function", fn);
+ }
const name = fn.name || "anonymous";Per coding guidelines: "Validate function arguments using validators from internal/validators and throw $ERR_* error codes for invalid arguments".
🤖 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 142 - 143, Validate the timerify
argument immediately: in timerify(fn: ...) call the appropriate validator from
internal/validators (e.g., validators.validateFunction(fn, 'fn') or
validators.checkFunction) at the start of the method and throw the corresponding
Node-style ERR_* (e.g., ERR_INVALID_ARG_TYPE) for non-callables instead of
deferring validation to the returned wrapper; ensure you import the validator
and perform the check before computing name or returning the wrapper so
non-callable inputs error synchronously.
| return function (...args: unknown[]) { | ||
| const start = performance.now(); | ||
| try { | ||
| const result = fn.apply(this, args); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use .$apply() in this builtin wrapper.
fn.apply(this, args) is user-tamperable in this layer. Switch to fn.$apply(this, args) to match the runtime safety rules for builtin JS modules.
Proposed fix
- const result = fn.apply(this, args);
+ const result = fn.$apply(this, args);As per coding guidelines, "Use .$call() and .$apply() instead of .call() and .apply() to prevent user tampering with function invocation".
📝 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 148, Replace the direct, tamperable
invocation fn.apply(this, args) with the runtime-safe built-in invocation
fn.$apply(this, args) in the wrapper that calls the user function (the line
creating const result). Locate the wrapper where fn is invoked (the const result
= ... statement) and change the call to use .$apply so the builtin module
follows the runtime safety rules for builtin JS modules.
| // Handle both sync and async functions | ||
| if (result && typeof result.then === "function") { | ||
| // Async function | ||
| return result.then((value: unknown) => { | ||
| const end = performance.now(); | ||
| performance.measure(`${name}()`, start as unknown as string, end as unknown as string); | ||
| return value; | ||
| }); | ||
| } else { | ||
| // Sync function | ||
| const end = performance.now(); | ||
| performance.measure(`${name}()`, start as unknown as string, end as unknown as string); | ||
| return result; | ||
| } | ||
| } catch (error) { | ||
| const end = performance.now(); | ||
| performance.measure(`${name}()`, start as unknown as string, end as unknown as string); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js perf_hooks, what entryTypeis emitted byperformance.timerify(), and should PerformanceObserver.observe({ entryTypes: ['function'] }) receive those entries?
💡 Result:
performance.timerify() (aka perf_hooks.timerify()) creates PerformanceEntry records with entryType === 'function' (Node calls this “Timerify ('function')”). When you have a PerformanceObserver observing entryTypes: ['function'], it should receive those timerify-generated entries. (nodejs.org)
Citations:
🏁 Script executed:
# First, let's look at the PerformanceEntry.cpp file to see what entry types are supported
find . -name "PerformanceEntry.cpp" | head -5Repository: oven-sh/bun
Length of output: 105
🏁 Script executed:
# Check what entry types are recognized in the PerformanceEntry implementation
rg -A 20 "entryType.*function\|function.*entryType" --type cpp src/bun.js/bindings/webcore/Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Search for performance.measure implementation to see if it has special handling for timerify context
rg "measure.*function\|timerify" --type ts --type js src/js/Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Read the PerformanceEntry.cpp file, particularly around lines 75-94
cat -n src/bun.js/bindings/webcore/PerformanceEntry.cpp | sed -n '70,100p'Repository: oven-sh/bun
Length of output: 976
🏁 Script executed:
# Search for the timerify implementation in perf_hooks.ts
cat -n src/js/node/perf_hooks.ts | sed -n '140,175p'Repository: oven-sh/bun
Length of output: 1652
🏁 Script executed:
# Look for where entry types are defined and handled in PerformanceEntry
rg "measure\|mark\|function" src/bun.js/bindings/webcore/PerformanceEntry.cpp -B 2 -A 2Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check the context around line 148 to see if .apply() is used and if there's validation
cat -n src/js/node/perf_hooks.ts | sed -n '142,170p'Repository: oven-sh/bun
Length of output: 1346
🏁 Script executed:
# Check if there's any validation in the timerify function or nearby
rg "validators\|ERR_" src/js/node/perf_hooks.ts -B 3 -A 3Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for $apply or .$apply usage patterns in other builtin files to confirm guideline
rg "\\\$apply|\\.\\$apply" src/js/builtins/ src/js/node/ -B 2 -A 2 | head -30Repository: oven-sh/bun
Length of output: 175
🏁 Script executed:
# Search for proper .$apply usage pattern
rg '\.\$apply' src/js/builtins/ src/js/node/ -B 2 -A 2 | head -40Repository: oven-sh/bun
Length of output: 1866
🏁 Script executed:
# Also check for $apply usage
rg '\$apply' src/js/builtins/ src/js/node/ -B 2 -A 2 | head -40Repository: oven-sh/bun
Length of output: 2268
🏁 Script executed:
# Check if fn is callable before being invoked - look for validation patterns
rg '\$isCallable' src/js/builtins/ src/js/node/ -B 2 -A 2 | head -20Repository: oven-sh/bun
Length of output: 794
🏁 Script executed:
# Check what happens if the async function rejects - is there error handling?
cat -n src/js/node/perf_hooks.ts | sed -n '150,170p'Repository: oven-sh/bun
Length of output: 1030
🏁 Script executed:
# Check the performance.measure() signature and overloads
rg "measure.*startMark.*endMark\|measure.*function" src/js/ src/bun.js/bindings/ -B 3 -A 3 | head -50Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for performance.measure() implementation to understand valid signatures
rg "measure\s*\(" src/js/node/perf_hooks.ts -B 2 -A 2Repository: oven-sh/bun
Length of output: 857
timerify() implementation has multiple issues preventing Node.js compatibility.
The entry type mismatch is confirmed: performance.measure() creates "measure" entries, but Node.js timerify() requires "function" entries that PerformanceObserver.observe({ entryTypes: ["function"] }) should receive. Bun's PerformanceEntry.cpp does not recognize "function" as a valid entry type.
Additionally:
- Line 148: Use
fn.$apply(this, args)instead offn.apply()per coding guidelines - Missing
$isCallable(fn)validation before invocation performance.measure()is called with numeric values cast to strings (lines 155, 161, 166), but the API expects mark names or an options object- Async promise rejection (line 153) has no
.catch()handler, so measurements are skipped and errors propagate uncaught
🤖 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 150 - 166, The timerify wrapper must
validate the callable, use fn.$apply(this, args), create proper start/end marks
(not numeric casts) and ensure measurements emit a "function" entry type and
handle promise rejections; specifically, in timerify: first check
$isCallable(fn) before invoking, call fn.$apply(this, args) to execute, create
unique start/end mark names (e.g., `${name}-start-${id}` / `${name}-end-${id}`)
and call performance.mark for each, then call performance.measure(name,
startMark, endMark) (not passing numeric timestamps), and for async results
attach both .then and .catch to record the end mark/measure in both fulfillment
and rejection paths; if the environment supports constructing custom
PerformanceEntry of type "function", prefer emitting that entry type for
PerformanceObserver.observe({ entryTypes: ["function"] }), otherwise fall back
to creating a "measure" entry, and ensure all marks/measures are cleared
afterward.
| if (result && typeof result.then === "function") { | ||
| // Async function | ||
| return result.then((value: unknown) => { | ||
| const end = performance.now(); | ||
| performance.measure(`${name}()`, start as unknown as string, end as unknown as string); | ||
| return value; | ||
| }); |
There was a problem hiding this comment.
Rejected Promises currently skip instrumentation.
The async branch only records in .then(). If fn returns a Promise that rejects, no timing entry is emitted, so async error cases diverge from the sync catch path. Record the end time in a finalizer-equivalent that runs for both fulfillment and rejection.
🤖 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 - 157, The async branch only
records timing on fulfillment; change the Promise handling for the case where
(result && typeof result.then === "function") so the end time is recorded for
both fulfillment and rejection. Replace the current result.then(value => { ...
}) usage with a finalizer-style handler (either result.finally(...) plus a then
to preserve value/error, or use result.then(value => { record end/measure;
return value }, err => { record end/measure; throw err })) so that
performance.measure(`${name}()` , start, end) is executed on both resolve and
reject while preserving the original return value or rethrowing the original
error; keep referencing the same symbols: result, name, start,
performance.measure.
| performance.measure(`${name}()`, start as unknown as string, end as unknown as string); | ||
| return value; | ||
| }); | ||
| } else { | ||
| // Sync function | ||
| const end = performance.now(); | ||
| performance.measure(`${name}()`, start as unknown as string, end as unknown as string); | ||
| return result; | ||
| } | ||
| } catch (error) { | ||
| const end = performance.now(); | ||
| performance.measure(`${name}()`, start as unknown as string, end as unknown as string); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What overloads does performance.measure()support in the Web Performance API / Node.js, and can numeric timestamps fromperformance.now() be passed by casting them to string arguments?
💡 Result:
Web Performance API (browsers) — supported overloads
Per User Timing Level 3, the signature is: (w3.org)
performance.measure(measureName)performance.measure(measureName, startMark)performance.measure(measureName, startMark, endMark)performance.measure(measureName, measureOptions)wheremeasureOptionscan include:start: number (DOMHighResTimeStamp) or string mark nameend: number or string mark nameduration: number (with the spec’s validity rules)detail: any structured-cloneable value (developer.mozilla.org)
Node.js (node:perf_hooks) — supported overloads
Node documents: (nodejs.org)
performance.measure(name[, startMarkOrOptions[, endMark]])
Where:
startMarkOrOptionsis either:- a string (start mark name), or
- an options object with
start/endas number|string, plus optionaldurationanddetail
endMarkis an optional string, and must be omitted ifstartMarkOrOptionsis an options object (nodejs.org)
Can you pass performance.now() timestamps by casting them to string arguments?
No—string arguments are interpreted as mark names, not numeric timestamps. So performance.measure("x", String(t0), String(t1)) will try to find marks literally named like "123.456" and typically throws because they don’t exist. (w3.org)
If you want to use numeric timestamps from performance.now(), pass them as numbers via the options form:
- Browser/Node:
performance.measure("x", { start: t0, end: t1 })(developer.mozilla.org)
Citations:
- 1: https://www.w3.org/TR/2021/WD-user-timing-3-20210913/
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Performance/measure
- 3: https://nodejs.org/api/perf_hooks.html
- 4: https://nodejs.org/api/perf_hooks.html
- 5: https://www.w3.org/TR/2021/WD-user-timing-3-20210913/
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Performance/measure
🏁 Script executed:
cat -n src/js/node/perf_hooks.ts | sed -n '140,175p'Repository: oven-sh/bun
Length of output: 1652
performance.measure() is being called with the wrong overload and will throw at runtime.
Lines 155, 161, and 166 cast numeric timestamps from performance.now() to strings and pass them as mark names. Since the Web Performance API interprets string arguments as literal mark names (not timestamps), these calls will fail trying to find marks named "123.456", etc., which don't exist. Use the options object form instead: performance.measure(name, { start, end }) with numeric start and end values.
🤖 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 155 - 166, The calls to
performance.measure are using the wrong overload by casting numeric timestamps
to strings; replace those calls to use the options-object overload so numeric
start/end values are passed (e.g., call performance.measure(name, { start, end
})) instead of performance.measure(`${name}()`, start as unknown as string, end
as unknown as string); update every occurrence (the async branch where value is
awaited, the sync branch returning result, and the catch block) to pass the
numeric start and end variables via the { start, end } object and remove the
string casts.
Adds missing 'hasSubscribers' property to TracingChannel interface. This property was added in oven-sh#27921.
2 participants
Fixes #9271
Adds performance.timerify() method to wrap functions and automatically
measure their execution time using Performance API.
Changes
Example
Testing
The implementation ensures: