-
Notifications
You must be signed in to change notification settings - Fork 5k
feat(perf_hooks): add performance.timerify() implementation #27921
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -139,6 +139,35 @@ export default { | |||||
| setResourceTimingBufferSize(_) { | ||||||
| return performance.setResourceTimingBufferSize(...arguments); | ||||||
| }, | ||||||
| timerify(fn: (...args: unknown[]) => unknown) { | ||||||
| const name = fn.name || "anonymous"; | ||||||
|
Comment on lines
+142
to
+143
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: If
🏁 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
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 🤖 Prompt for AI Agents |
||||||
|
|
||||||
| return function (...args: unknown[]) { | ||||||
| const start = performance.now(); | ||||||
| try { | ||||||
| const result = fn.apply(this, args); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Use
Proposed fix- const result = fn.apply(this, args);
+ const result = fn.$apply(this, args);As per coding guidelines, "Use 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
|
|
||||||
| // 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; | ||||||
| }); | ||||||
|
Comment on lines
+151
to
+157
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rejected Promises currently skip instrumentation. The async branch only records in 🤖 Prompt for AI Agents |
||||||
| } 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); | ||||||
|
Comment on lines
+150
to
+166
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result:
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
The entry type mismatch is confirmed: Additionally:
🤖 Prompt for AI Agents
Comment on lines
+155
to
+166
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Web Performance API (browsers) — supported overloadsPer User Timing Level 3, the signature is: (w3.org)
Node.js (
|
||||||
| throw error; | ||||||
| } | ||||||
| }; | ||||||
| }, | ||||||
| timeOrigin: performance.timeOrigin, | ||||||
| toJSON(_) { | ||||||
| return performance.toJSON(...arguments); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the cleanup example non-throwing.
This snippet will throw
ENOENTwhen one of the sidecar files was already removed or never created, which is one of the cases this note is describing. Useforce: trueor an existence check so the example works cross-platform.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents