feat: add memory leak detection script for react benchmark#2247
feat: add memory leak detection script for react benchmark#2247hzy wants to merge 2 commits intolynx-family:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a leak-detection workflow to the React benchmark: a new script that analyzes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
🦋 Changeset detectedLatest commit: 1f915ce The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR adds a memory leak detection script for React benchmarks to help identify memory leaks by analyzing Perfetto trace files (.ptrace). The script queries trace data for FiberElement constructor/destructor events and reports any elements that were constructed but never destroyed.
Changes:
- Added
detectLeak.mjsscript to analyze .ptrace files for memory leaks in React benchmarks - Added
@lynx-js/trace-processordependency (v0.0.1) to parse and query trace files - Added
perfetto:detect-leaknpm script to run the leak detection tool
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| benchmark/react/scripts/detectLeak.mjs | New script that analyzes .ptrace files to detect memory leaks by comparing FiberElement constructors vs destructors |
| benchmark/react/package.json | Adds @lynx-js/trace-processor dev dependency and perfetto:detect-leak script |
| pnpm-lock.yaml | Lockfile updates for new @lynx-js/trace-processor dependency and its transitive dependency immer |
| .changeset/wild-jeans-take.md | Changeset file for documenting the changes (currently empty) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@benchmark/react/package.json`:
- Line 23: The JSON in the package's "scripts" section has trailing commas
(e.g., after the "perfetto:detect-leak" entry) which violates our dprint rules;
remove any trailing commas after object entries and after the closing brace of
objects in benchmark/react/package.json so the file is valid JSON (ensure the
"scripts" object entries remain in the same order and that
"perfetto:detect-leak" is the last entry without a trailing comma).
🧹 Nitpick comments (1)
benchmark/react/scripts/detectLeak.mjs (1)
55-60: Earlyprocess.exit(1)prevents analysis of remaining files.If the first
.ptracefile lacks constructor events, the script terminates immediately without analyzing subsequent files. The rest of the script uses ahasLeakflag pattern (Lines 111, 117–118) for deferred exit. Consider aligning this case for consistency.♻️ Suggested refactor
if (countIt.valid()) { const count = countIt.get('count'); if (count === 0) { console.error( `Error: No FiberElement::Constructor events found in ${file}.`, ); - process.exit(1); // eslint-disable-line n/no-process-exit + hasLeak = true; + continue; } }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@benchmark/react/package.json`:
- Line 38: The devDependency "@lynx-js/trace-processor" referenced in
package.json does not exist and will break installs; either remove the
"@lynx-js/trace-processor" entry from devDependencies or add/restore the package
as a workspace package or published npm package and update package.json to point
to the correct version or workspace reference (e.g., workspace: or valid
version) so the dependency resolves during installation.
In `@examples/react-compiler/package.json`:
- Line 19: The package.json currently pins the dependency
"@rsbuild/plugin-babel" to an alpha pre-release "2.0.0-alpha.3"; replace this
unstable version with a stable release (either "1.1.0" — latest stable — or
revert to "1.0.6" if the PR isn't intended to change babel plugin behavior).
Update the version string for the "@rsbuild/plugin-babel" entry in package.json
accordingly and run your package manager (npm/yarn/pnpm) to reinstall and update
lockfile.
hzy
force-pushed
the
feat/react-leak-detection
branch
from
804271b to
b89b3e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@benchmark/react/package.json`:
- Line 14: The "bench:detect-leak" script will be picked up by the "bench"
aggregate (the "bench" npm script matches /^bench:.*/), causing detect-leak to
run when traces don't exist; rename the script key to "perfetto:detect-leak" (or
another "perfetto:*" name) so it runs after perfetto traces are produced, or
alternatively update the "bench" aggregate pattern to exclude
"bench:detect-leak"; locate the "bench:detect-leak" entry in package.json and
either change its key to "perfetto:detect-leak" or modify the aggregate script
pattern to prevent matching it.
🧹 Nitpick comments (1)
benchmark/react/scripts/detectLeak.mjs (1)
44-61: Earlyprocess.exit(1)skips remaining.ptracefiles.If one file has zero
FiberElement::Constructorevents, the script exits immediately without analyzing the rest. If this is intentional (treat missing constructors as a fatal trace error), consider adding a brief comment explaining the rationale. Otherwise, you could accumulate errors and exit after processing all files, consistent with how leak detection is handled below.
hzy
force-pushed
the
feat/react-leak-detection
branch
from
54a8c86 to
980caec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@benchmark/react/scripts/detectLeak.mjs`:
- Around line 40-61: The abrupt process.exit(1) inside the scope where "using
engine = new WasmEngine('detectLeak')" prevents the engine's Symbol.dispose from
running; replace the direct process.exit(1) in the block that handles count ===
0 with throwing a descriptive Error (e.g., throw new Error(`No
FiberElement::Constructor events found in ${file}`)) or set a failure flag and
break so the using block can unwind and dispose the WasmEngine; ensure the
thrown error is caught or allowed to propagate to the existing top-level handler
(the call site that currently exits near line 118/122) so the program still
terminates non-zero but after proper disposal.
- Around line 40-42: The file uses the `using` resource-management syntax with
`using engine = new WasmEngine('detectLeak')` which Node.js 22 (V8 v12.4) does
not support; replace that pattern with explicit construction and manual cleanup:
instantiate WasmEngine (new WasmEngine('detectLeak')), run await
engine.parse(fileContent) and await engine.notifyEof() inside a try block, and
in a finally block call the engine's cleanup method (e.g., engine.dispose(),
engine.close(), or engine.free() — whichever `WasmEngine` exposes) to release
resources, or alternatively add transpilation/polyfills for TC39 Explicit
Resource Management if you choose to keep `using`. Ensure you reference the
WasmEngine constructor and its cleanup method when making the change.
🧹 Nitpick comments (1)
benchmark/react/scripts/detectLeak.mjs (1)
122-122: Top-levelawait main()will produce an unhandled rejection on failure.If
main()throws (e.g., from afs.readFileerror or the WASM engine), the rejection is unhandled at the top level. Consider adding a.catch()to log the error and set a non-zero exit code explicitly.Proposed fix
-await main(); +await main().catch((err) => { + console.error(err); + process.exit(1); // eslint-disable-line n/no-process-exit +});
| using engine = new WasmEngine('detectLeak'); | ||
| await engine.parse(fileContent); | ||
| await engine.notifyEof(); | ||
|
|
||
| const countQuery = ` | ||
| SELECT count(*) as count | ||
| FROM slice s | ||
| JOIN args a1 ON s.arg_set_id = a1.arg_set_id | ||
| JOIN args a2 ON s.arg_set_id = a2.arg_set_id | ||
| WHERE s.name = 'FiberElement::Constructor' AND a1.key = 'debug.id' AND a2.key = 'debug.tag' | ||
| `; | ||
| const countResult = await engine.query(countQuery); | ||
| const countIt = countResult.iter({ count: 0 }); | ||
| if (countIt.valid()) { | ||
| const count = countIt.get('count'); | ||
| if (count === 0) { | ||
| console.error( | ||
| `Error: No FiberElement::Constructor events found in ${file}.`, | ||
| ); | ||
| process.exit(1); // eslint-disable-line n/no-process-exit | ||
| } | ||
| } |
There was a problem hiding this comment.
process.exit(1) bypasses using disposal — WASM engine may not be cleaned up.
process.exit() at line 59 forces an abrupt termination that does not unwind the scope, so the Symbol.dispose registered by using engine on line 40 will never fire. While the OS reclaims memory on exit, the WASM engine may need graceful shutdown (e.g., flushing internal state).
Replace process.exit(1) with a thrown error (or set a flag and break), so the using block disposes the engine normally before the outer process.exit(1) on line 118.
Proposed fix
if (countIt.valid()) {
const count = countIt.get('count');
if (count === 0) {
console.error(
`Error: No FiberElement::Constructor events found in ${file}.`,
);
- process.exit(1); // eslint-disable-line n/no-process-exit
+ throw new Error(
+ `No FiberElement::Constructor events found in ${file}.`,
+ );
}
}Then catch the error at the call site (line 122) or let it propagate as a non-zero exit.
📝 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.
| using engine = new WasmEngine('detectLeak'); | |
| await engine.parse(fileContent); | |
| await engine.notifyEof(); | |
| const countQuery = ` | |
| SELECT count(*) as count | |
| FROM slice s | |
| JOIN args a1 ON s.arg_set_id = a1.arg_set_id | |
| JOIN args a2 ON s.arg_set_id = a2.arg_set_id | |
| WHERE s.name = 'FiberElement::Constructor' AND a1.key = 'debug.id' AND a2.key = 'debug.tag' | |
| `; | |
| const countResult = await engine.query(countQuery); | |
| const countIt = countResult.iter({ count: 0 }); | |
| if (countIt.valid()) { | |
| const count = countIt.get('count'); | |
| if (count === 0) { | |
| console.error( | |
| `Error: No FiberElement::Constructor events found in ${file}.`, | |
| ); | |
| process.exit(1); // eslint-disable-line n/no-process-exit | |
| } | |
| } | |
| using engine = new WasmEngine('detectLeak'); | |
| await engine.parse(fileContent); | |
| await engine.notifyEof(); | |
| const countQuery = ` | |
| SELECT count(*) as count | |
| FROM slice s | |
| JOIN args a1 ON s.arg_set_id = a1.arg_set_id | |
| JOIN args a2 ON s.arg_set_id = a2.arg_set_id | |
| WHERE s.name = 'FiberElement::Constructor' AND a1.key = 'debug.id' AND a2.key = 'debug.tag' | |
| `; | |
| const countResult = await engine.query(countQuery); | |
| const countIt = countResult.iter({ count: 0 }); | |
| if (countIt.valid()) { | |
| const count = countIt.get('count'); | |
| if (count === 0) { | |
| console.error( | |
| `Error: No FiberElement::Constructor events found in ${file}.`, | |
| ); | |
| throw new Error( | |
| `No FiberElement::Constructor events found in ${file}.`, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@benchmark/react/scripts/detectLeak.mjs` around lines 40 - 61, The abrupt
process.exit(1) inside the scope where "using engine = new
WasmEngine('detectLeak')" prevents the engine's Symbol.dispose from running;
replace the direct process.exit(1) in the block that handles count === 0 with
throwing a descriptive Error (e.g., throw new Error(`No
FiberElement::Constructor events found in ${file}`)) or set a failure flag and
break so the using block can unwind and dispose the WasmEngine; ensure the
thrown error is caught or allowed to propagate to the existing top-level handler
(the call site that currently exits near line 118/122) so the program still
terminates non-zero but after proper disposal.
| using engine = new WasmEngine('detectLeak'); | ||
| await engine.parse(fileContent); | ||
| await engine.notifyEof(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js 22 explicit resource management using keyword support runtime
💡 Result:
Node.js v22 does not support JavaScript’s Explicit Resource Management syntax (using / await using) at runtime.
- The feature (“Explicit Resource Management”) ships in V8 v13.8 (Chrome 134). (v8.dev)
- Node.js v22.x uses V8 v12.4 (example: Node v22.4.0 ships with V8 v12.4.254.21), which is earlier than the required V8 version—so
usingwill be a syntax error in Node 22. (nodejs.org) - A quick compatibility summary: Node 22 → V8 12.4 (no
using), while Node 24 → V8 13.6 (addsusing). (samthor.au)
What to do on Node 22
- Transpile
using/await using(e.g., TypeScript/Babel), and polyfill the runtime pieces (Symbol.dispose,Symbol.asyncDispose,DisposableStack,AsyncDisposableStack,SuppressedError). (typescriptlang.org) - Or avoid the syntax and just use a polyfill/shim for
DisposableStack-style APIs (syntax still won’t work without transpilation). (core-js.io)
Citations:
- 1: https://v8.dev/features/explicit-resource-management?utm_source=openai
- 2: https://nodejs.org/en/download/archive/v22.4.0?utm_source=openai
- 3: https://samthor.au/node-releases/?utm_source=openai
- 4: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html?utm_source=openai
- 5: https://core-js.io/docs/features/proposals/explicit-resource-management?utm_source=openai
Replace using syntax or add transpilation for Node.js 22 compatibility.
The using keyword (TC39 Explicit Resource Management) requires V8 v13.8+. Node.js 22 ships with V8 v12.4 and does not support this syntax—it will fail at runtime. Node.js 24 (V8 v13.6+) supports it. Since this repo targets Node.js ^22 || ^24, either:
- Transpile this file with TypeScript/Babel and polyfill the runtime (
DisposableStack,Symbol.dispose, etc.), or - Use a try-finally pattern or manual resource cleanup instead of
using, or - Drop Node.js 22 support
🤖 Prompt for AI Agents
In `@benchmark/react/scripts/detectLeak.mjs` around lines 40 - 42, The file uses
the `using` resource-management syntax with `using engine = new
WasmEngine('detectLeak')` which Node.js 22 (V8 v12.4) does not support; replace
that pattern with explicit construction and manual cleanup: instantiate
WasmEngine (new WasmEngine('detectLeak')), run await engine.parse(fileContent)
and await engine.notifyEof() inside a try block, and in a finally block call the
engine's cleanup method (e.g., engine.dispose(), engine.close(), or
engine.free() — whichever `WasmEngine` exposes) to release resources, or
alternatively add transpilation/polyfills for TC39 Explicit Resource Management
if you choose to keep `using`. Ensure you reference the WasmEngine constructor
and its cleanup method when making the change.
1c2fe84 to
59924eb
Compare
hzy
force-pushed
the
feat/react-leak-detection
branch
from
59924eb to
fb84b6d
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
Web Explorer#7686 Bundle Size — 383.74KiB (0%).1f915ce(current) vs df4bc6e main#7677(baseline) Bundle metrics
|
| Current #7686 |
Baseline #7677 |
|
|---|---|---|
 Initial JS |
154.88KiB |
154.88KiB |
| Initial CSS |
35.06KiB |
35.06KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
239 |
239 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.99% |
2.99% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7686 |
Baseline #7677 |
|
|---|---|---|
| JS |
252.83KiB |
252.83KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.06KiB |
35.06KiB |
Bundle analysis report Branch hzy:feat/react-leak-detection Project dashboard
Generated by RelativeCI Documentation Report issue
1 participant
This PR adds a memory leak detection script for React benchmarks.
Summary by CodeRabbit
New Features
Chores