perf: Refactor factorize dependency tracking#13306
Conversation
github-actions
bot
added
the
team
|
📝 Benchmark detail: Open
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10c45d3684
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| dependencies: self.dependencies, | ||
| issuer: self.issuer, | ||
| issuer_identifier: self.original_module_identifier, | ||
| dependencies: vec![dependency], |
There was a problem hiding this comment.
This refactor now calls the module factory with dependencies: vec![dependency], so only the primary dependency is visible to factory hooks and plugins. That regresses behavior for grouped dependencies: for example, ContextReplacementPlugin iterates data.dependencies to clear critical on each context dependency, and finish_modules later reads diagnostics from each dependency in the module graph, so non-primary duplicates can now keep stale critical diagnostics and emit warnings that were previously suppressed. The grouped dependency set still needs to be propagated (or hook-side mutations applied back to every grouped dependency id), not just the first entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 059baef. process_dependencies still groups by DependencyId, but FactorizeTask now rebuilds and passes the full dependency list into ModuleFactoryCreateData, so hook-side mutations on data.dependencies are preserved and synced back to the module graph.
There was a problem hiding this comment.
Pull request overview
This PR refactors the build-module-graph “repair” pipeline to treat a group of equivalent dependencies as a single “primary” dependency plus a list of associated DependencyIds, updating task wiring to use the new grouped representation.
Changes:
- Introduce
ModuleDependenciesto carry a primaryBoxDependencyalongside groupedDependencyIds. - Update factorize/process/add/overwrite/entry tasks to use
ModuleDependenciesAPIs (primary_id,primary_dependency, etc.). - Change dependency grouping in
ProcessDependenciesTaskto accumulate ids intoModuleDependenciesinstead of collectingVec<BoxDependency>.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/rspack_core/src/compilation/build_module_graph/module_executor/overwrite.rs | Switch dependency id extraction to ModuleDependencies::primary_id() for tracker callbacks. |
| crates/rspack_core/src/compilation/build_module_graph/module_executor/entry.rs | Construct factorize tasks using ModuleDependencies::from_dependency. |
| crates/rspack_core/src/compilation/build_module_graph/graph_updater/repair/process_dependencies.rs | Group dependencies into ModuleDependencies keyed by resource identifier. |
| crates/rspack_core/src/compilation/build_module_graph/graph_updater/repair/mod.rs | Update repair entry-dependency factorization to use ModuleDependencies. |
| crates/rspack_core/src/compilation/build_module_graph/graph_updater/repair/factorize.rs | Add ModuleDependencies and refactor factorize/factorize-result to track grouped ids. |
| crates/rspack_core/src/compilation/build_module_graph/graph_updater/repair/add.rs | Update add flow to resolve modules for all grouped ids and compute forwarded ids via module graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let request = dependency | ||
| .as_module_dependency() | ||
| .map(|d| d.request().to_string()) | ||
| .or_else(|| { | ||
| self.dependencies[0] | ||
| dependency | ||
| .as_context_dependency() | ||
| .map(|d| d.request().to_string()) | ||
| }) | ||
| .unwrap_or_default(); | ||
| let (dependency, dependency_ids) = dependencies.into_parts(); | ||
| // Error and result are not mutually exclusive in webpack module factorization. | ||
| // Rspack puts results that need to be shared in both error and ok in [ModuleFactoryCreateData]. | ||
| let mut create_data = ModuleFactoryCreateData { | ||
| compiler_id: self.compiler_id, | ||
| compilation_id: self.compilation_id, | ||
| resolve_options: self.resolve_options, | ||
| options: self.options.clone(), | ||
| compiler_id, | ||
| compilation_id, | ||
| resolve_options, | ||
| options: options.clone(), | ||
| context, | ||
| request, | ||
| dependencies: self.dependencies, | ||
| issuer: self.issuer, | ||
| issuer_identifier: self.original_module_identifier, | ||
| dependencies: vec![dependency], | ||
| issuer, |
There was a problem hiding this comment.
FactorizeTask now passes only a single “primary” dependency into ModuleFactoryCreateData (dependencies: vec![dependency]) and later reconstructs ModuleDependencies by taking only the first element from create_data.dependencies. This changes semantics for grouped dependencies: module/context module factory hooks (e.g. ContextReplacementPlugin’s ContextModuleFactoryBeforeResolve) currently iterate and mutate every dependency in data.dependencies; with this change those mutations will only apply to one dependency, and any additional dependencies inserted/returned by hooks will be dropped. To preserve existing hook behavior, keep passing the full dependency list through module factory/hook execution and then sync all mutated dependencies back into the module graph (while still tracking the grouped DependencyIds).
There was a problem hiding this comment.
Fixed in 059baef. FactorizeTask no longer drops grouped dependencies before module-factory hooks. The full dependency vector flows through create_data.dependencies again, and FactorizeResultTask/AddTask sync all mutated dependencies back into the module graph.
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 1.88KB from 49.01MB to 49.01MB (⬆️0.00%) |
hardfist
changed the title
github-actions
bot
added
the
release: performance
Merging this PR will not alter performance
Comparing 
Footnotes
|
Deploying rspack with
|
| Latest commit: |
e7ab482
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://85ba7705.rspack-v2.pages.dev |
| Branch Preview URL: | https://codex-change-factorize-task.rspack-v2.pages.dev |
Summary
Testing