perf(layout): Use unit_id, not pkg hash, for bin/lib pkg_dirs for new layout #16345
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,7 +92,8 @@ impl fmt::Debug for UnitHash { | |
| pub struct Metadata { | ||
| unit_id: UnitHash, | ||
| c_metadata: UnitHash, | ||
| c_extra_filename: bool, | ||
| pkg_dir: bool, | ||
| } | ||
|
|
||
| impl Metadata { | ||
|
|
@@ -108,7 +109,12 @@ impl Metadata { | |
|
|
||
| /// A hash to add to file names through `-C extra-filename` | ||
| pub fn c_extra_filename(&self) -> Option<UnitHash> { | ||
| self.c_extra_filename.then_some(self.unit_id) | ||
| } | ||
|
|
||
| /// A hash to add to Cargo directory names | ||
| pub fn pkg_dir(&self) -> Option<UnitHash> { | ||
| self.pkg_dir.then_some(self.unit_id) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -245,11 +251,11 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> { | |
| }; | ||
| let name = unit.pkg.package_id().name(); | ||
| let meta = self.metas[unit]; | ||
| let hash = meta | ||
| .pkg_dir() | ||
| .map(|h| h.to_string()) | ||
| .unwrap_or_else(|| self.target_short_hash(unit)); | ||
| format!("{name}{separator}{hash}") | ||
| } | ||
|
|
||
| /// Returns the final artifact path for the host (`/…/target/debug`) | ||
|
|
@@ -681,7 +687,8 @@ fn compute_metadata( | |
| .iter() | ||
| .map(|dep| *metadata_of(&dep.unit, build_runner, metas)) | ||
| .collect::<Vec<_>>(); | ||
| let c_extra_filename = use_extra_filename(bcx, unit); | ||
| let pkg_dir = use_pkg_dir(bcx, unit); | ||
|
|
||
| let mut shared_hasher = StableHasher::new(); | ||
|
|
||
|
|
@@ -784,42 +791,37 @@ fn compute_metadata( | |
| dep_c_metadata_hashes.sort(); | ||
| dep_c_metadata_hashes.hash(&mut c_metadata_hasher); | ||
|
|
||
| let mut unit_id_hasher = shared_hasher.clone(); | ||
| // Mix in the target-metadata of all the dependencies of this target. | ||
| let mut dep_unit_id_hashes = deps_metadata.iter().map(|m| m.unit_id).collect::<Vec<_>>(); | ||
| dep_unit_id_hashes.sort(); | ||
| dep_unit_id_hashes.hash(&mut unit_id_hasher); | ||
| // Avoid trashing the caches on RUSTFLAGS changing via `unit_id` | ||
| // | ||
| // Limited to `unit_id` to help with reproducible build / PGO issues. | ||
| let default = Vec::new(); | ||
| let extra_args = build_runner.bcx.extra_args_for(unit).unwrap_or(&default); | ||
| if !has_remap_path_prefix(&extra_args) { | ||
| extra_args.hash(&mut unit_id_hasher); | ||
| } | ||
| if unit.mode.is_doc() || unit.mode.is_doc_scrape() { | ||
| if !has_remap_path_prefix(&unit.rustdocflags) { | ||
| unit.rustdocflags.hash(&mut unit_id_hasher); | ||
| } | ||
| } else { | ||
| if !has_remap_path_prefix(&unit.rustflags) { | ||
| unit.rustflags.hash(&mut unit_id_hasher); | ||
| } | ||
| } | ||
|
|
||
| let c_metadata = UnitHash(Hasher::finish(&c_metadata_hasher)); | ||
| let unit_id = UnitHash(Hasher::finish(&unit_id_hasher)); | ||
|
|
||
| Metadata { | ||
| unit_id, | ||
| c_metadata, | ||
| c_extra_filename, | ||
| pkg_dir, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -922,3 +924,51 @@ fn use_extra_filename(bcx: &BuildContext<'_, '_>, unit: &Unit) -> bool { | |
| } | ||
| true | ||
| } | ||
|
|
||
| /// Returns whether or not this unit should use a hash in the pkg_dir to make it unique. | ||
| fn use_pkg_dir(bcx: &BuildContext<'_, '_>, unit: &Unit) -> bool { | ||
| if unit.mode.is_doc_test() || unit.mode.is_doc() { | ||
| // Doc tests do not have metadata. | ||
| return false; | ||
| } | ||
| if bcx.gctx.cli_unstable().build_dir_new_layout { | ||
| // These always use metadata. | ||
| return true; | ||
| } | ||
|
Comment on lines
+934
to
+937
Member
There was a problem hiding this comment. There is no test showing the behavior change. Should we add some no-metadata test cases for both old and new layout, so at least new layout will show it start having metadata
Contributor
Author
There was a problem hiding this comment. The change here is from When we stabilize |
||
| if unit.mode.is_any_test() || unit.mode.is_check() { | ||
| // These always use metadata. | ||
| return true; | ||
| } | ||
| // No metadata in these cases: | ||
| // | ||
| // - dylibs: | ||
| // - if any dylib names are encoded in executables, so they can't be renamed. | ||
| // - TODO: Maybe use `-install-name` on macOS or `-soname` on other UNIX systems | ||
| // to specify the dylib name to be used by the linker instead of the filename. | ||
| // - Windows MSVC executables: The path to the PDB is embedded in the | ||
| // executable, and we don't want the PDB path to include the hash in it. | ||
| // - wasm32-unknown-emscripten executables: When using emscripten, the path to the | ||
| // .wasm file is embedded in the .js file, so we don't want the hash in there. | ||
| // | ||
| // This is only done for local packages, as we don't expect to export | ||
| // dependencies. | ||
| // | ||
| // The __CARGO_DEFAULT_LIB_METADATA env var is used to override this to | ||
| // force metadata in the hash. This is only used for building libstd. For | ||
| // example, if libstd is placed in a common location, we don't want a file | ||
| // named /usr/lib/libstd.so which could conflict with other rustc | ||
| // installs. In addition it prevents accidentally loading a libstd of a | ||
| // different compiler at runtime. | ||
| // See https://github.com/rust-lang/cargo/issues/3005 | ||
| let short_name = bcx.target_data.short_name(&unit.kind); | ||
| if (unit.target.is_dylib() | ||
| || unit.target.is_cdylib() | ||
| || (unit.target.is_executable() && short_name == "wasm32-unknown-emscripten") | ||
| || (unit.target.is_executable() && short_name.contains("msvc"))) | ||
| && unit.pkg.package_id().source_id().is_path() | ||
| && bcx.gctx.get_env("__CARGO_DEFAULT_LIB_METADATA").is_err() | ||
| { | ||
| return false; | ||
| } | ||
| true | ||
| } | ||
|
Comment on lines
928
to
974
Member
There was a problem hiding this comment.
What kind of breakages did you see?
Contributor
Author
|
||
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine for now until we want to store rustdoc artifacts separately.
A future idea, with
-Zrustdoc-mergeable-info, we could keep the each rustdoc artifacts separately. And later we do a merge thatrustdoc --merge=finalizethenA downside of it: we'll need to copy a lot of files. Probably not worthy