perf: Optimize ModuleGraph API implementation#13010
Conversation
There was a problem hiding this comment.
Pull request overview
Refines ModuleGraph batch update APIs to avoid parallel cloning/reinsertion and instead mutate connections/modules in-place, aiming to reduce overhead in module graph operations.
Changes:
- Remove Rayon-based parallel mapping in several
ModuleGraphbatch update methods. - Update connections and module graph modules in-place via
*_mutaccessors to avoid cloning and re-inserting.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (dep_id, original_module_identifier) in tasks { | ||
| let con = self | ||
| .connection_by_dependency_id_mut(&dep_id) | ||
| .expect("should have connection"); | ||
| con.original_module_identifier = Some(original_module_identifier); |
There was a problem hiding this comment.
expect("should have connection") will panic without identifying which DependencyId was missing, making debugging difficult. Consider using unwrap_or_else/expect with a message that includes dep_id so failures in batch updates are actionable.
| for (dep_id, module_identifier) in tasks { | ||
| let con = self | ||
| .connection_by_dependency_id_mut(&dep_id) | ||
| .expect("should have connection"); | ||
| con.set_module_identifier(module_identifier); |
There was a problem hiding this comment.
Same as above: this expect("should have connection") panic message lacks the DependencyId context. Including dep_id in the panic/expect message would make failures easier to diagnose (similar to do_update_module nearby).
📦 Binary Size-limit
🎉 Size decreased by 48.00KB from 48.65MB to 48.61MB (⬇️0.10%) |
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
Summary
Testing