refactor(core): drop redundant per-call FileId/path indices in detectors#357
Merged
Conversation
find_unused_exports, find_private_type_leaks, and find_duplicate_exports each rebuilt a FxHashMap (reachable_files set, module_by_id, module_idx_by_file_id, file_id_by_path) on every invocation. FileIds are assigned sequentially in discover/walk.rs:264, so graph.modules can be indexed directly via file_id.0 as usize, the same pattern find_unused_files already uses at unused_files.rs:73. Real-world wall-clock is in the noise across next.js (125k files), typescript (44k), vite (24k), vue-core (12k), and svelte (11k). Issue counts on next.js identical (24986). Shipped as the structural-cleanup branch of the issue's acceptance criterion. No behavior change. Fixes #333
Follow-up to 6349779 addressing panel-review feedback: 1. Document the invariant on ModuleGraph::modules: modules[file_id.0 as usize].file_id == file_id, anchored to discover/walk.rs and build::populate_edges. Single source of truth instead of duplicating the rationale in three detector comments. 2. Add debug_assert_eq! at each of the four index-by-FileId sites introduced in the parent commit (find_unused_exports cross-file ref check, find_private_type_leaks module lookup, find_duplicate_exports enumerate, collect_dynamic_reexport_sources wrapper lookup). Fires under cargo test if a future ModuleGraph::build change ever filters or sparsifies modules silently. Zero cost in release. 3. Confirmed issue-count parity on all five real-world fixtures (was verified on next.js only in the parent commit): next.js 24986, typescript 14622, vite 1582, vue-core 239, svelte 4855, identical BEFORE/AFTER. Refs #333
Collaborator
Author
|
Released in v2.73.0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes four redundant per-call hash-map builds in
crates/core/src/analyze/:reachable_files: FxHashSet<u32>infind_unused_exportsmodule_by_id: FxHashMap<FileId, &ModuleNode>infind_private_type_leaksmodule_idx_by_file_id: FxHashMap<FileId, usize>infind_duplicate_exportsfile_id_by_path: FxHashMap<&Path, FileId>infind_duplicate_exportsFileIds are assigned sequentially in
crates/core/src/discover/walk.rs:264after path-sorting, sograph.modules[file_id.0 as usize]is correct. This is the same patternfind_unused_filesalready uses atcrates/core/src/analyze/unused_files.rs:73. The maps were doing redundant work.Net: -79 +41 lines on a single file.
Behavior
No change. Issue counts identical on
benchmarks/fixtures/real-world/next.js(24986 issues, BEFORE and AFTER).Timing
Wall-clock is in-the-noise across five fixtures (warm cache, multiple runs):
Per issue #333's acceptance criterion: "ships as a pure structural cleanup if the timing is in-the-noise."
Test plan
cargo test --workspace --all-targets(37 test suites green)cargo clippy --workspace --all-targets -- -D warnings(clean)cargo fmt --all -- --check(clean)cargo doc --workspace --no-deps --document-private-items(clean)Fixes #333