test: add dev server benchmark for page route generation#34323
test: add dev server benchmark for page route generation#34323
Conversation
|
|
danielroe
changed the title
danielroe
changed the title
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
WalkthroughA new benchmark test file is added to packages/nuxt/test that simulates a Nuxt dev-server route-generation workflow. It defines medium and large sets of page file paths, constructs a simulator that rebuilds the route tree on every operation via Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 2
🤖 Fix all issues with AI agents
In `@packages/nuxt/test/pages-dev.bench.ts`:
- Around line 62-65: The helper makeFiles creates file objects but sets
absolutePath to the relative string p (e.g. "pages/about.vue") which is
incorrect; update makeFiles to produce a real absolute path (use Node's
path.resolve or path.join with process.cwd() or the repo root) for absolutePath
so generateRoutesFromFiles and any path comparisons get proper absolute values;
ensure path is imported/required and replace the absolutePath: p assignment in
makeFiles with something like path.resolve(p) (or path.resolve(root, p)) to fix
identity/comparison issues.
- Around line 99-107: Bench callbacks are including reset calls so the timed
iterations measure both the operation and its reset (sim.addFile/
sim.removeFile), contaminating results; remove the in-bench reset calls (the
sim.removeFile(newFile) in the "add file + emit" bench and the sim.addFile(...)
reset in the "remove file + emit" bench) or move them into proper setup/teardown
hooks (Vitest bench setup/teardown or describe beforeEach/afterEach) so each
bench only measures the intended operation (use the same change for the large
app suite benches that reference mediumAppPaths/largeAppPaths).
| const makeFiles = (paths: string[]) => paths.map(p => ({ | ||
| relativePath: p.replace(/^pages\//, ''), | ||
| absolutePath: p, | ||
| })) |
There was a problem hiding this comment.
absolutePath is set to a relative path.
absolutePath: p assigns values like "pages/about.vue" which are not absolute paths. If generateRoutesFromFiles ever resolves or compares absolute paths (e.g. for file identity), this could produce incorrect results. For a benchmark this likely doesn't affect timing, but it's worth being aware of if results look unexpected.
🤖 Prompt for AI Agents
In `@packages/nuxt/test/pages-dev.bench.ts` around lines 62 - 65, The helper
makeFiles creates file objects but sets absolutePath to the relative string p
(e.g. "pages/about.vue") which is incorrect; update makeFiles to produce a real
absolute path (use Node's path.resolve or path.join with process.cwd() or the
repo root) for absolutePath so generateRoutesFromFiles and any path comparisons
get proper absolute values; ensure path is imported/required and replace the
absolutePath: p assignment in makeFiles with something like path.resolve(p) (or
path.resolve(root, p)) to fix identity/comparison issues.
| bench('add file + emit', () => { | ||
| sim.addFile(newFile) | ||
| sim.removeFile(newFile) // reset | ||
| }) | ||
|
|
||
| bench('remove file + emit', () => { | ||
| sim.removeFile(mediumAppPaths[Math.floor(mediumAppPaths.length / 2)]!) | ||
| sim.addFile(mediumAppPaths[Math.floor(mediumAppPaths.length / 2)]!) // reset | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reset operations inside bench callbacks are included in the measured time.
The removeFile (reset) call on line 101 is executed within the timed iteration, so each benchmark iteration measures "add + remove" rather than just "add + emit". Same issue on lines 105–106 for the remove benchmark. This contaminates the measurements, making add and remove benchmarks appear roughly equivalent and ~2× their true cost.
Vitest bench supports setup/teardown hooks (or you could use the beforeEach/afterEach pattern at the describe level). Alternatively, since the simulator is stateless (it rebuilds from scratch each call), the reset calls are actually no-ops in terms of mutating allFiles — addFile/removeFile don't mutate filePaths or allFiles, they create new arrays. So the resets are unnecessary and just add noise.
Proposed fix — remove unnecessary resets
bench('add file + emit', () => {
sim.addFile(newFile)
- sim.removeFile(newFile) // reset
})
bench('remove file + emit', () => {
sim.removeFile(mediumAppPaths[Math.floor(mediumAppPaths.length / 2)]!)
- sim.addFile(mediumAppPaths[Math.floor(mediumAppPaths.length / 2)]!) // reset
})And similarly for the large app suite (lines 122–130).
📝 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.
| bench('add file + emit', () => { | |
| sim.addFile(newFile) | |
| sim.removeFile(newFile) // reset | |
| }) | |
| bench('remove file + emit', () => { | |
| sim.removeFile(mediumAppPaths[Math.floor(mediumAppPaths.length / 2)]!) | |
| sim.addFile(mediumAppPaths[Math.floor(mediumAppPaths.length / 2)]!) // reset | |
| }) | |
| bench('add file + emit', () => { | |
| sim.addFile(newFile) | |
| }) | |
| bench('remove file + emit', () => { | |
| sim.removeFile(mediumAppPaths[Math.floor(mediumAppPaths.length / 2)]!) | |
| }) |
🤖 Prompt for AI Agents
In `@packages/nuxt/test/pages-dev.bench.ts` around lines 99 - 107, Bench callbacks
are including reset calls so the timed iterations measure both the operation and
its reset (sim.addFile/ sim.removeFile), contaminating results; remove the
in-bench reset calls (the sim.removeFile(newFile) in the "add file + emit" bench
and the sim.addFile(...) reset in the "remove file + emit" bench) or move them
into proper setup/teardown hooks (Vitest bench setup/teardown or describe
beforeEach/afterEach) so each bench only measures the intended operation (use
the same change for the large app suite benches that reference
mediumAppPaths/largeAppPaths).
Merging this PR will not alter performance
Performance Changes
Comparing 
Footnotes |
1 participant
🔗 Linked issue
#34316
📚 Description
this adds a benchmark we can use to compare with new incremental model for route generation for #34316