Conversation
github-actions
bot
added
release: performance
|
📝 Benchmark detail: Open
|
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
📦 Binary Size-limit
❌ Size increased by 8.25KB from 48.55MB to 48.56MB (⬆️0.02%) |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a module federation performance regression by parallelizing the batch processing of modules in the FlagDependencyUsagePlugin.
Changes:
- Replaced sequential batch processing with concurrent execution using
rspack_futures::scope - Refactored
process_modulefrom an instance method to a static method to enable parallel execution - Added unsafe code block for concurrent task spawning with proper safety comments
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
| .await | ||
| .into_iter() | ||
| .map(|res| res.expect("flag dependency usage task failed")) |
There was a problem hiding this comment.
Using .expect() will cause a panic if any task fails, which will crash the entire compilation process. Consider handling errors more gracefully, such as collecting errors and reporting them as diagnostics instead of panicking. This would provide better error messages and allow the compilation to continue or fail more gracefully.
| .map(|res| res.expect("flag dependency usage task failed")) | |
| .filter_map(|res| match res { | |
| Ok(value) => Some(value), | |
| Err(err) => { | |
| eprintln!("flag dependency usage task failed: {:?}", err); | |
| None | |
| } | |
| }) |
| compilation | ||
| .plugin_driver | ||
| .compilation_hooks | ||
| .dependency_referenced_exports | ||
| .call( | ||
| self.compilation, | ||
| compilation, | ||
| &dep_id, | ||
| &referenced_exports_result, | ||
| runtime, | ||
| Some(self.build_module_graph_artifact.get_module_graph()), | ||
| Some(module_graph), | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
Potential race condition: The dependency_referenced_exports hook is being called concurrently from multiple parallel tasks. While the hook itself is defined as Series (sequential execution), multiple tasks spawned by rspack_futures::scope are calling this hook simultaneously. This could lead to race conditions if the hook has any internal state or if it performs operations that aren't thread-safe. Consider whether this hook needs to be called sequentially across all tasks, or if it's safe to be called concurrently.
2 participants
Summary
partial fix perf regression introduced in #12250
make process_modules parallel which is supported by rayon in previous implemantion
Related links
Checklist