fix: avoid runtime scope conflicts in module factories - #15002
Conversation
📦 Binary Size-limit
🎉 Size decreased by 4.00KB from 66.77MB to 66.77MB (⬇️0.01%) |
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
Merging this PR will improve performance by 3.05%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | rust@persistent_cache_restore_after_single_file_change@basic-react-development |
4.8 ms | 4.7 ms | +3.15% |
| ⚡ | Simulation | rust@persistent_cache_restore@basic-react-development |
4.5 ms | 4.3 ms | +3.03% |
| ⚡ | Simulation | rust@create_chunk_assets |
1.5 ms | 1.4 ms | +2.96% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing seal/fix-cjs-rspack-require-conflict (2c6e7c9) with main (c05244a)
Footnotes
-
47 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
f2e74c9 to
37a4f32
Compare
976e2cf to
c15397c
Compare
c15397c to
cdb2c2f
Compare
cdb2c2f to
81a1022
Compare
81a1022 to
88a4f49
Compare
60f9f26 to
43da34f
Compare
f398886 to
8709506
Compare
8709506 to
43d561f
Compare
a6c846d to
86a2eda
Compare
|
@codspeedbot explain the regression |
|
| Run | Branch commit | Value |
|---|---|---|
base (main) |
5a52b06 |
45.27 ms |
| PR run 14:21 | ffd0ce2 |
45.20 ms |
| PR run 15:28 | 5b647b5 |
45.24 ms |
| PR run 15:58 (reported) | e8a7061 |
47.32 ms |
The earlier PR runs are flat against main (even marginally faster). Comparing 5b647b5 → e8a7061 directly in CodSpeed reproduces the exact same -4.39% on this benchmark, on matching runners (no environment-difference warning). The complete diff between those two commits is:
crates/rspack_plugin_devtool/src/eval_dev_tool_module_plugin.rs | 2 ++
crates/rspack_plugin_devtool/src/eval_source_map_dev_tool_plugin.rs | 2 ++
crates/rspack_plugin_javascript/src/plugin/api_plugin.rs | 2 ++
crates/rspack_plugin_javascript/src/plugin/drive.rs | 2 ++
crates/rspack_plugin_javascript/src/plugin/url_plugin.rs | 2 ++
i.e. five #![allow(clippy::too_many_arguments)] attributes — a lint-only change that cannot make code slower. Simulation mode reports instruction counts, so what shifted is the machine code rustc emitted for the bench binary (inlining/codegen-unit decisions in rspack_plugin_javascript), not the work performed.
2. The flame graphs confirm a codegen artifact
Comparing base (45.3 ms) and head (47.3 ms) flame graphs:
ConcatenatedModule::code_generationsubtree: 25.7 ms in both, self time 951.0 µs → 950.5 µs — flat.- The entire delta sits in the spawned per-module task subtree: 18.6 → 20.6 ms, but its own self time is flat (862.3 → 861.9 µs). It propagates down to
NormalModule::code_generation(7.8 → 9.3 ms) andJavaScriptParserAndGenerator::generate→source_dependency(6.9 → 8.4 ms). - Meanwhile
memcpyis identical (2.8 ms), allocator self times are flat, and an untouched function,rspack_core::escape_identifier_impl, actually got cheaper (756.7 → 688.1 µs) with identical call counts.
A subtree getting 20 % more expensive while its own self time and its siblings stay bit-stable, and unrelated functions moving in the opposite direction, is the signature of a re-compiled binary — not of extra work.
3. The changed code does not execute in this benchmark
The benchmark measures the code-generation stage only. The PR's changes live in chunk rendering (JavascriptModulesRenderModuleContent now carrying runtime_requirements), runtime module templates (RuntimeCodeTemplate dojang decoupling) and the SRI plugin — none of which appear anywhere in this flame graph. The one changed function that is on the code-generation path, ModuleCodeTemplate::render_runtime_scope (called once per module from ParserRuntimeRequirementsData::new), is O(1) and now resolves the render map with a direct match instead of a helper call; it was already present in the 5b647b5 run that measured 45.24 ms.
Recommendation
Nothing to fix here. A re-run (or the next push) should settle back around 45.2–45.3 ms; if you want it off the report now, acknowledging it on CodSpeed is the right call.
Summary
- Warming up the performance wizard...
- Investigating the concatenate_module_code_generation regression — Regression localized and attributed to a build/codegen artifact, not to the PR's rendering changes
- Handling comment — Comment handling finished successfully
Learnings updated. See what CodSpeed remembers about this repository.
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
86a2eda to
2c6e7c9
Compare
Deploying rspack with
|
| Latest commit: |
2c6e7c9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2fc1412a.rspack-v2.pages.dev |
| Branch Preview URL: | https://seal-fix-cjs-rspack-require.rspack-v2.pages.dev |
Summary
rspackRequireas the outer scope forRspackExportfactories while avoiding redundant or conflicting factory parametersRuntimeCodeTemplateand retain only the rendered require name in the compilation contextrspackRequirebinding conflicts and modern-module SRI async chunksRelated links
Testing
cargo clippy --workspace --all-targets --tests --locked -- -D warningscorepack pnpm --filter @rspack/binding run build:devEsmOutputandRuntimeModeEsmOutputcases forcjs-rspack-requireandmodern-module-sriChecklist