feat(benchmarks): add memory benchmarks#7585
Open
Sheraff wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a memory benchmarking system: shared result utilities and CLI, two React scenarios (client repeated-navigation and SSR repeated-requests) with Vite/Playwright configs, a GitHub Actions workflow to run and publish results, and package/tsconfig/gitignore wiring. ChangesMemory Benchmarks Setup and Execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit bae5e40
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview2 package(s) bumped directly, 7 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
benchmarks/memory/tools/result-utils.ts (1)
274-290: 💤 Low valueSimplify redundant type check.
Lines 282-283 check both
typeof result.metrics.peakRssBytes === 'number'andNumber.isFinite(result.metrics.peakRssBytes). TheNumber.isFinite()check already returnsfalsefor non-number types, making thetypeofcheck redundant.♻️ Proposed simplification
extra: [ `slope=${Math.round(result.metrics.retainedHeapSlopeBytesPerOperation)} B/op`, `peak_heap=${Math.round(result.metrics.peakHeapUsedBytes)} B`, - typeof result.metrics.peakRssBytes === 'number' && - Number.isFinite(result.metrics.peakRssBytes) + Number.isFinite(result.metrics.peakRssBytes) ? `peak_rss=${Math.round(result.metrics.peakRssBytes)} B` : undefined, ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/memory/tools/result-utils.ts` around lines 274 - 290, In toBenchmarkAction, simplify the conditional that builds the extra array by removing the redundant typeof check for result.metrics.peakRssBytes and only use Number.isFinite(result.metrics.peakRssBytes) to guard the `peak_rss=` entry; update the ternary that currently reads `typeof ... === 'number' && Number.isFinite(...) ? ... : undefined` to just `Number.isFinite(result.metrics.peakRssBytes) ? \`peak_rss=...\` : undefined`, referencing the result.metrics.peakRssBytes and toBenchmarkAction symbols.benchmarks/memory/package.json (2)
28-28: 💤 Low valueConsider using caret range for
@types/nodeto match other type packages.The
@types/nodepackage is pinned to exact version25.0.9without a caret, while other type packages like@types/reactand@types/react-domuse caret ranges. For consistency and to allow patch updates, consider using^25.0.9.♻️ Suggested consistency improvement
- "`@types/node`": "25.0.9", + "`@types/node`": "^25.0.9",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/memory/package.json` at line 28, The package.json dependency entry for "`@types/node`" is pinned to "25.0.9" while other type packages use caret ranges; update the "`@types/node`" version string to use a caret range (e.g., "^25.0.9") so it matches the style of "`@types/react`" and "`@types/react-dom`" and allows patch/minor updates while preserving major version compatibility.
32-32: TypeScript ^6.0.2 availability is OK; confirm it’s the intended benchmark target
TypeScript 6.0.2 exists on npm, so the dependency is resolvable. Confirm the project intentionally targets the 6.x line for these benchmarks (and that the caret range^6.0.2is desired).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/memory/package.json` at line 32, The package.json dependency "typescript": "^6.0.2" should be explicitly verified as the intended benchmark target; either confirm and document that benchmarks target the 6.x line (e.g., update the repo README or a benchmarks/README note) or change the dependency to the desired semver (pin to "6.0.2" if exact, or broaden/narrow the range) in package.json so the intent is clear; update any CI/test configs that assume a different TypeScript major version accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/memory/scenarios/react/client/repeated-navigation/src/main.tsx`:
- Around line 160-164: The navigateAndWait function's options parameter is typed
as any — replace it with the proper NavigateOptions type exported by TanStack
Router: update the function signature navigateAndWait(options: NavigateOptions)
and add an import for the type (import type { NavigateOptions } from
'`@tanstack/router`'); ensure any call sites still pass the same object shape and
adjust or cast them only if necessary.
---
Nitpick comments:
In `@benchmarks/memory/package.json`:
- Line 28: The package.json dependency entry for "`@types/node`" is pinned to
"25.0.9" while other type packages use caret ranges; update the "`@types/node`"
version string to use a caret range (e.g., "^25.0.9") so it matches the style of
"`@types/react`" and "`@types/react-dom`" and allows patch/minor updates while
preserving major version compatibility.
- Line 32: The package.json dependency "typescript": "^6.0.2" should be
explicitly verified as the intended benchmark target; either confirm and
document that benchmarks target the 6.x line (e.g., update the repo README or a
benchmarks/README note) or change the dependency to the desired semver (pin to
"6.0.2" if exact, or broaden/narrow the range) in package.json so the intent is
clear; update any CI/test configs that assume a different TypeScript major
version accordingly.
In `@benchmarks/memory/tools/result-utils.ts`:
- Around line 274-290: In toBenchmarkAction, simplify the conditional that
builds the extra array by removing the redundant typeof check for
result.metrics.peakRssBytes and only use
Number.isFinite(result.metrics.peakRssBytes) to guard the `peak_rss=` entry;
update the ternary that currently reads `typeof ... === 'number' &&
Number.isFinite(...) ? ... : undefined` to just
`Number.isFinite(result.metrics.peakRssBytes) ? \`peak_rss=...\` : undefined`,
referencing the result.metrics.peakRssBytes and toBenchmarkAction symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 544b5d15-aa37-4ef5-a368-71ea28b7f992
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.github/workflows/memory-benchmarks.ymlbenchmarks/memory/.gitignorebenchmarks/memory/README.mdbenchmarks/memory/package.jsonbenchmarks/memory/results/.gitignorebenchmarks/memory/results/scenarios/.gitkeepbenchmarks/memory/scenarios/react/client/repeated-navigation/index.htmlbenchmarks/memory/scenarios/react/client/repeated-navigation/playwright.config.tsbenchmarks/memory/scenarios/react/client/repeated-navigation/src/main.tsxbenchmarks/memory/scenarios/react/client/repeated-navigation/tests/memory.spec.tsbenchmarks/memory/scenarios/react/client/repeated-navigation/tsconfig.jsonbenchmarks/memory/scenarios/react/client/repeated-navigation/vite.config.tsbenchmarks/memory/scenarios/react/ssr/repeated-requests/bench.tsbenchmarks/memory/scenarios/react/ssr/repeated-requests/src/routeTree.gen.tsbenchmarks/memory/scenarios/react/ssr/repeated-requests/src/router.tsxbenchmarks/memory/scenarios/react/ssr/repeated-requests/src/routes/$a.$b.$c.$d.tsxbenchmarks/memory/scenarios/react/ssr/repeated-requests/src/routes/$a.$b.$c.tsxbenchmarks/memory/scenarios/react/ssr/repeated-requests/src/routes/$a.$b.tsxbenchmarks/memory/scenarios/react/ssr/repeated-requests/src/routes/$a.tsxbenchmarks/memory/scenarios/react/ssr/repeated-requests/src/routes/__root.tsxbenchmarks/memory/scenarios/react/ssr/repeated-requests/src/workload.tsxbenchmarks/memory/scenarios/react/ssr/repeated-requests/tsconfig.jsonbenchmarks/memory/scenarios/react/ssr/repeated-requests/vite.config.tsbenchmarks/memory/tools/report.tsbenchmarks/memory/tools/result-utils.tsbenchmarks/memory/tools/summary.tsbenchmarks/memory/tools/tsconfig.jsonbenchmarks/memory/tsconfig.jsonpackage.json
Comment on lines
+160
to
+164
| async function navigateAndWait(options: any) { | ||
| const rendered = waitForRendered() | ||
| await router.navigate(options) | ||
| await rendered | ||
| } |
Contributor
There was a problem hiding this comment.
Replace any type with proper NavigateOptions type.
The options parameter uses any, which violates the TypeScript strict mode guideline. TanStack Router exports a NavigateOptions type that should be used instead.
🔧 Proposed fix
+import type { NavigateOptions } from '`@tanstack/react-router`'
+
// ... rest of imports ...
-async function navigateAndWait(options: any) {
+async function navigateAndWait(options: NavigateOptions) {
const rendered = waitForRendered()
await router.navigate(options)
await rendered
}📝 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
| async function navigateAndWait(options: any) { | |
| const rendered = waitForRendered() | |
| await router.navigate(options) | |
| await rendered | |
| } | |
| async function navigateAndWait(options: NavigateOptions) { | |
| const rendered = waitForRendered() | |
| await router.navigate(options) | |
| await rendered | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/memory/scenarios/react/client/repeated-navigation/src/main.tsx`
around lines 160 - 164, The navigateAndWait function's options parameter is
typed as any — replace it with the proper NavigateOptions type exported by
TanStack Router: update the function signature navigateAndWait(options:
NavigateOptions) and add an import for the type (import type { NavigateOptions }
from '`@tanstack/router`'); ensure any call sites still pass the same object shape
and adjust or cast them only if necessary.
Source: Coding guidelines
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We investigated this failure and determined it is unrelated to the PR's changes, which only add memory benchmark infrastructure and register a new workspace package. The failing SolidStart e2e test concerns Unicode routing behavior that is not touched by any file in this diff. We are classifying this as a pre-existing environment issue that should be re-run or investigated independently.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
benchmarks/memoryVerification
pnpm install --lockfile-onlyCI=1 NX_DAEMON=false pnpm nx run @benchmarks/memory:test:types --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false MEMORY_BENCH_ITERATIONS=5 MEMORY_BENCH_WARMUP_ITERATIONS=2 MEMORY_BENCH_BATCH_SIZE=5 pnpm nx run @benchmarks/memory:test:memory --outputStyle=stream --skipRemoteCache --parallel=2CI=1 NX_DAEMON=false pnpm nx run @benchmarks/memory:report --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit
New Features
Chores