perf: Disable incremental tracking when build module graph incremental is off#13272
perf: Disable incremental tracking when build module graph incremental is off#13272
Conversation
hardfist
changed the title
github-actions
bot
added
release: performance
|
📝 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: 16288cb08c
ℹ️ 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".
| const incrementalEnabled = | ||
| this.compiler.options.incremental !== false; |
There was a problem hiding this comment.
This gate treats any non-false incremental config as "incremental dependency tracking is available", but users can set incremental: { buildModuleGraph: false, ... }. In that mode this patch rebuilds the module graph from scratch each run, so the internal __internal__added*/__internal__removed* sets are not reliable deltas; using them here means removed dependencies are never unwatched and stale paths keep accumulating in watch mode. Please branch on whether the build-module-graph pass is enabled (or fall back to #diffDependencies) instead of only checking incremental !== false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR ensures incremental module-graph tracking/caching is disabled (and bookkeeping reset) when the incremental build module graph pass is turned off, including watch/HMR scenarios, and adds a regression test to prevent cache reuse in that mode.
Changes:
- Stop persisting module graph artifacts/snapshots when
BUILD_MODULE_GRAPHincremental is disabled. - Reset/disable incremental dependency tracking and related counters when incremental is disabled.
- Add a cacheCase regression test covering “incremental-disabled + persistent cache”.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rspack-test/cacheCases/common/update-file-incremental-disabled/rspack.config.js | Adds a persistent-cache config with incremental: false + plugin assertions for the regression case |
| tests/rspack-test/cacheCases/common/update-file-incremental-disabled/loader.js | Loader records processed files to validate rebuild behavior |
| tests/rspack-test/cacheCases/common/update-file-incremental-disabled/index.js | Test flow covering HMR + multiple restarts with incremental disabled |
| tests/rspack-test/cacheCases/common/update-file-incremental-disabled/file.js | Multi-step fixture to drive rebuild expectations across runs |
| packages/rspack/src/Watching.ts | Computes dependency diffs manually when incremental tracking is disabled |
| crates/rspack_core/src/utils/incremental_info.rs | Adds an enabled switch and disable() to stop tracking and reset state |
| crates/rspack_core/src/utils/file_counter/mod.rs | Exposes a way to disable incremental info tracking for file counters |
| crates/rspack_core/src/stats/mod.rs | Adjusts “module built” logic when incremental tracking is disabled |
| crates/rspack_core/src/compilation/mod.rs | Changes dependency iterators to avoid incremental-only bookkeeping when disabled |
| crates/rspack_core/src/compilation/build_module_graph/module_executor/mod.rs | Disables incremental tracking / resets artifacts when the build-module-graph pass is disabled |
| crates/rspack_core/src/compilation/build_module_graph/mod.rs | Rebuilds entry dependencies into a fresh artifact when the pass is disabled |
| crates/rspack_core/src/cache/persistent/snapshot/mod.rs | Adds Snapshot::replace() helper to overwrite a scope |
| crates/rspack_core/src/cache/persistent/mod.rs | Skips restoring/saving module-graph cache when the incremental pass is disabled |
| crates/rspack_core/src/artifacts/build_module_graph_artifact.rs | Adds disable_incremental_tracking() to turn off incremental bookkeeping in artifacts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .build_module_graph_artifact | ||
| .context_dependencies | ||
| .added_files() | ||
| .chain(&self.file_dependencies), |
There was a problem hiding this comment.
In the incremental-enabled branches for context_dependencies(), missing_dependencies(), and build_dependencies(), added_files() is chained with &self.file_dependencies rather than the corresponding dependency set (&self.context_dependencies, &self.missing_dependencies, &self.build_dependencies). This produces incorrect “added” iterators (mixing in file deps) and likely breaks downstream snapshot/build-deps updates. Chain each “added” iterator with its matching self.*_dependencies collection (consistent with the all_files and incremental-disabled branches).
| .chain(&self.file_dependencies), | |
| .chain(&self.context_dependencies), |
| let added_files = if self.incremental.enabled() { | ||
| Either::Left( | ||
| self | ||
| .build_module_graph_artifact | ||
| .missing_dependencies | ||
| .added_files() | ||
| .chain(&self.file_dependencies), | ||
| ) |
There was a problem hiding this comment.
In the incremental-enabled branches for context_dependencies(), missing_dependencies(), and build_dependencies(), added_files() is chained with &self.file_dependencies rather than the corresponding dependency set (&self.context_dependencies, &self.missing_dependencies, &self.build_dependencies). This produces incorrect “added” iterators (mixing in file deps) and likely breaks downstream snapshot/build-deps updates. Chain each “added” iterator with its matching self.*_dependencies collection (consistent with the all_files and incremental-disabled branches).
| let added_files = if self.incremental.enabled() { | ||
| Either::Left( | ||
| self | ||
| .build_module_graph_artifact | ||
| .build_dependencies | ||
| .added_files() | ||
| .chain(&self.file_dependencies), | ||
| ) |
There was a problem hiding this comment.
In the incremental-enabled branches for context_dependencies(), missing_dependencies(), and build_dependencies(), added_files() is chained with &self.file_dependencies rather than the corresponding dependency set (&self.context_dependencies, &self.missing_dependencies, &self.build_dependencies). This produces incorrect “added” iterators (mixing in file deps) and likely breaks downstream snapshot/build-deps updates. Chain each “added” iterator with its matching self.*_dependencies collection (consistent with the all_files and incremental-disabled branches).
| process.nextTick(() => { | ||
| if (!this.#closed) { | ||
| const incrementalEnabled = | ||
| this.compiler.options.incremental !== false; |
There was a problem hiding this comment.
This gating uses compiler.options.incremental !== false, but the rest of this PR’s behavior is keyed off whether the BUILD_MODULE_GRAPH incremental pass is enabled. If the pass is disabled while incremental remains enabled, this code will still rely on compilation.__internal__added/removed* (which may be empty/meaningless in that mode), potentially causing the watcher to miss dependency changes. Consider plumbing a dedicated flag that reflects passes_enabled(BUILD_MODULE_GRAPH) into the JS compilation object and using that here (or otherwise switching to the diff-based path when the pass is disabled).
| this.compiler.options.incremental !== false; | |
| this.compiler.options.incremental !== false && | |
| compilation.__internal__addedFileDependencies != null && | |
| compilation.__internal__removedFileDependencies != null && | |
| compilation.__internal__addedContextDependencies != null && | |
| compilation.__internal__removedContextDependencies != null && | |
| compilation.__internal__addedMissingDependencies != null && | |
| compilation.__internal__removedMissingDependencies != null; |
|
|
||
| if (incrementalEnabled) { | ||
| fileDependencies.added = new Set( | ||
| compilation.__internal__addedFileDependencies, | ||
| ); | ||
| fileDependencies.removed = new Set( | ||
| compilation.__internal__removedFileDependencies, | ||
| ); | ||
| contextDependencies.added = new Set( | ||
| compilation.__internal__addedContextDependencies, | ||
| ); | ||
| contextDependencies.removed = new Set( | ||
| compilation.__internal__removedContextDependencies, | ||
| ); | ||
| missingDependencies.added = new Set( | ||
| compilation.__internal__addedMissingDependencies, | ||
| ); | ||
| missingDependencies.removed = new Set( | ||
| compilation.__internal__removedMissingDependencies, | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
This gating uses compiler.options.incremental !== false, but the rest of this PR’s behavior is keyed off whether the BUILD_MODULE_GRAPH incremental pass is enabled. If the pass is disabled while incremental remains enabled, this code will still rely on compilation.__internal__added/removed* (which may be empty/meaningless in that mode), potentially causing the watcher to miss dependency changes. Consider plumbing a dedicated flag that reflects passes_enabled(BUILD_MODULE_GRAPH) into the JS compilation object and using that here (or otherwise switching to the diff-based path when the pass is disabled).
| let build_module_graph_incremental_enabled = compilation | ||
| .incremental | ||
| .passes_enabled(IncrementalPasses::BUILD_MODULE_GRAPH); | ||
|
|
||
| if build_module_graph_incremental_enabled { | ||
| // save snapshot | ||
| // TODO add a all_dependencies to collect dependencies | ||
| let (_, file_added, file_updated, file_removed) = compilation.file_dependencies(); | ||
| let (_, context_added, context_updated, context_removed) = | ||
| compilation.context_dependencies(); | ||
| let (_, missing_added, missing_updated, missing_removed) = | ||
| compilation.missing_dependencies(); | ||
| let (_, build_added, build_updated, _) = compilation.build_dependencies(); | ||
| self | ||
| .build_deps | ||
| .add(build_added.chain(build_updated).cloned()) | ||
| .await, | ||
| ); | ||
| .snapshot | ||
| .remove(SnapshotScope::FILE, file_removed.cloned()); | ||
| self | ||
| .snapshot | ||
| .remove(SnapshotScope::CONTEXT, context_removed.cloned()); | ||
| self | ||
| .snapshot | ||
| .remove(SnapshotScope::MISSING, missing_removed.cloned()); | ||
| self | ||
| .snapshot | ||
| .add(SnapshotScope::FILE, file_added.chain(file_updated).cloned()) | ||
| .await; | ||
| self | ||
| .snapshot | ||
| .add( | ||
| SnapshotScope::CONTEXT, | ||
| context_added.chain(context_updated).cloned(), | ||
| ) | ||
| .await; | ||
| self | ||
| .snapshot | ||
| .add( | ||
| SnapshotScope::MISSING, | ||
| missing_added.chain(missing_updated).cloned(), | ||
| ) | ||
| .await; | ||
| self.warnings.extend( | ||
| self | ||
| .build_deps | ||
| .add(build_added.chain(build_updated).cloned()) | ||
| .await, | ||
| ); | ||
| } else { | ||
| let (build_all, _, _, _) = compilation.build_dependencies(); | ||
| self | ||
| .warnings | ||
| .extend(self.build_deps.add(build_all.cloned()).await); | ||
| } |
There was a problem hiding this comment.
In the build_module_graph_incremental_enabled == false branch, the cache still updates build_deps but does not clear/replace the snapshot scopes (FILE/CONTEXT/MISSING). That can leave stale snapshot data persisted on disk even though this mode intends to stop persisting/tracking module-graph-related caches, and it may also cause confusing state if the pass is later re-enabled. Consider explicitly clearing/replacing those snapshot scopes in the else branch (the newly added Snapshot::replace() looks well-suited here), or otherwise ensuring stale snapshot entries aren’t retained when the pass is disabled.
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 16.00KB from 48.98MB to 48.99MB (⬆️0.03%) |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Summary
Testing