Load only the crate header for locator::crate_matches
#111329
Conversation
|
r? @ozkanonur (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Depdep is binary dep info yeah, the flag is -Zbinary-dep-depinfo IIRC. My sense is that this has a chance of causing extra invalidation where it's not technically needed... maybe due to e.g. doc clearing out the full build directory or something? I don't have a great test case in mind though, I'm not sure why I(?) limited that originally. Looking at the past PRs in this area, I had cited this case:
Can you check if that breaks here? My guess is yes. In general since I wrote that on #86641 (comment), I feel like this case is less important, but would be good to have a clear sense of tradeoffs. Working builds are probably more important than slightly faster builds... That said, my sense is that something about the depinfo changing is still a better fix here, at least in the long run. Maybe now that we have a test case that's worth taking another look at. |
looks like the original PR adding this was #67760, and you actually had the same code from this PR until you found in #67760 (comment) that it broke the tests (same as in this PR 😄). I think that test failure is fixable while still keeping this change (maybe we're deleting the sysroot too aggressively?) - I'll look into it.
It does not :)
Hmm, ok - I thought bindep-depinfo was only for rustbuild, but looking at #63012 (comment) I see @jonhoo and others are using it for toolchain tracking too. I'll see if I can minimize the bug. |
|
Actually, it looks like binary-dep-depinfo doesn't even try to address this use case? Am I missing something? https://github.com/rust-lang/rust/blob/1fa1f5932b487a2ac4105deca26493bb8013a9a6/compiler/rustc_interface/src/passes.rs#L490-L511 |
|
|
|
I tried adding the toolchain to the dep-info file (i.e. diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs
index 6a94d19001e..a5ca209b2bf 100644
--- a/compiler/rustc_interface/src/passes.rs
+++ b/compiler/rustc_interface/src/passes.rs
@@ -517,6 +517,14 @@ fn write_out_deps(
files.push(escape_dep_filename(&path.display().to_string()));
}
}
+
+ match std::env::current_exe() {
+ Ok(driver_path) => files.push(escape_dep_filename(&driver_path.display().to_string())),
+ Err(e) => {
+ #[allow(rustc::untranslatable_diagnostic, rustc::diagnostic_outside_of_impl)] // old enough this changes a lot in later commits
+ sess.err(format!("failed to determine path to current process: {e}"));
+ }
+ }
}
let mut file = BufWriter::new(fs::File::create(&deps_filename)?); |
|
I think that test is actually misleading? You're not rebuilding the compiler in the first step, so we're not running the code here at all. You need to build through to the bin/rustc binary (I forget what Step does that). After that I still expect it to cause more re-compilations than needed.
I'm not sure I understand what this use case is. The expectation in my mind is that if you change rustc in a way that changes metadata format, you should get a new std. The binary depdep info will include the dependency on std, and so we will cause a rebuild of the compiler. I think that isn't true with keep-stage1-std or whatever that flag is, but I'm not sure that the fully right fix is trying to fix that... I guess we can make this conditional on that flag? Essentially if we've artificially kept any dependency Step the next one needs this special cleaning logic. (For example, keeping rustc would mean needing this logic for rustdoc, if I'm connecting the dots right?) Essentially keep-stage bypasses this logic by avoiding an update to the Std/Library binaries. But with #76720 (comment) I guess we're seeing this without keep-stage? I'm not sure what is particular about that test case...
I'm not sure about incremental, but why would metadata caches be a problem? rustc should never be loading metadata it's not compatible with because dependencies are built by Cargo before the next rustc runs. Looking at the test case in #76720 (comment) we're failing to recompile libserde_derive in stage1-tools for some reason, that's the file we fail to load: I'm going to try to figure out why we're not recompiling libserde_derive -- the .d file rustc emits for it points to a non-existent libstd file: |
|
https://gist.githubusercontent.com/Mark-Simulacrum/3e5d4dc8d659da79376d1181617d121a/raw/3b5176553de7ec6d0b7649bc1e0552c84dfee0f2/log is the log file including Cargo's fingerprint and rustc's metadata loading. I'm still trying to dig through that, no clear leads just yet. My latest suspicion is that Cargo actually does rebuild the serde_derive and serde crates (in an earlier invocation), but that when rustc goes to look for the libserde_derive crate it's failing to load that as it's loading an earlier version of that crate. (Cargo doesn't delete stale files in the target directory). I'm not quite sure yet why we see this only with serde-derive and not other (non proc macro?) dependencies... |
|
OK, so I think I have at least a theoretical understanding of what causes this. You need to go from version A to version B of rust-lang/rust where both of these are true (plus having built A):
The first condition is needed so that when rustc is run, it outputs a distinct/separate file (e.g., in this case we have version B with libserde_derive-67ce481370f50db2.so and version A with libserde_derive-5970f6e0b3790395.so). The second condition is needed so that when we go to load the metadata from each of these to get the full SVH and confirm which one is correct, we fail to deserialize the metadata at that time (this is what causes the ICE). I think a better fix than the one in this PR is to change our metadata format so that we can get at the SVH, crate name, etc. (i.e., everything accessed here: rust/compiler/rustc_metadata/src/locator.rs Lines 668 to 701 in 0dddad0 (I think it should even be possible to make the metadata version something we compute dynamically, e.g., by serializing a known set of information in this "CratePrefix" and then hashing that, which may be an alternative fix here). Either of these approaches would mean that the ICE hit in #76720 (comment) isn't hit anymore, while preserving the cache behavior we want (around libtest -> rustc_span not being a valid edge, for example). This should also make it much more rare that this happens outside of rustc tree, which I think is sometimes the case today, particularly where others also rely on -Zbinary-dep-depinfo, which we're hoping to stabilize "soon". @petrochenkov -- do you have thoughts on the metadata format change here? Preference for whether to change the way we compute the version constant or moving some information into a crateprefix struct or similar with more "stable" encoding (e.g., no optimization around laziness, etc.)? |
|
r? @bjorn3 |
|
I think changing the crate metadata format to include all information used by the That being said, even with that change we will still need to manually clear the incremental cache, but at least that shouldn't cause any unnecessary recompilations. Just slower recompilations when a recompilation is actually necessary. |
|
I think an easier solution for incremental is to just turn it off altogether for stage 1+ builds; it seems unlikely in practice that we'll ever actually be able to reuse the cache. |
|
With |
jyn514
added
S-waiting-on-author
this was exactly it!! here's a script to reproduce it (assuming you have a checkout at #!/bin/sh
unset CARGO_TARGET_DIR
cargo new repro-76720
cd repro-76720
cargo add serde_derive@1.0.162
build() {
worktree=$1
commit=$2
serde_version=$3
toolchain=$4
# We can't use RTIM because this only repros with `omit-git-hash`.
(cd ../$worktree && git checkout $commit && x build std proc_macro)
cargo update -p serde_derive --precise $serde_version
cargo clean -p repro-76720 -p serde_derive
cargo +$toolchain-stage1 build
}
build rust2 6874f4e3fc2a16be7c78e702d068bbc1daa90e16 1.0.162 r2
build rust3 999ac5f7770bff68bd65f490990d32c3ec1faaa6 1.0.163 r3going to work tomorrow on actually fixing the bug now that there's an MCVE :) ty again! |
|
I'm going to reuse METADATA_VERSION instead of adding a second version constant for now. I think it will be very unlikely in practice to need to change |
|
Some changes occurred in src/tools/rust-analyzer cc @rust-lang/rust-analyzer |
|
Do note that this will break rust-analyzer proc macro loading on nightly until this change is synced back and I believe there was some issue preventing syncs back right now. The rust-analyzer server attempts to read the rustc version from proc macros before spawning proc-macro-srv from the sysroot. |
|
No that should be fine. Those checks are only done at the proc-macro server side now, the rust-analyzer server doesn't check the dylibs itself anymore. |
|
@Veykril who handles subtree syncs? Will you or another RA member make a sync after this lands or should I do the sync myself? |
|
@lnicola usually does them, but we currently have a problem with syncing because it would break our CI atm. I forgot to look into that, I'll look into that again in a bit. |
|
|
|
I wonder if RA should get the rustc version from proc-macro-srv instead of trying to read it directly. Then it wouldn't be broken by these updates. The only time it needs the version is if it failed to load the proc-macro anyway, which should be very rare now that it's using the server in the sysroot. |
Well, it doesn't do that anymore on rust-lang/rust-analyzer, the subtree is just that outdated 😅 see rust-lang/rust-analyzer@3ae9bfe |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (99ff5af): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 643.87s -> 644.761s (0.14%) |
Load only the crate header for `locator::crate_matches` Previously, we used the following info to determine whether to load the crate: 1. The METADATA_HEADER, which includes a METADATA_VERSION constant 2. The embedded rustc version 3. Various metadata in the `CrateRoot`, including the SVH This worked ok most of the time. Unfortunately, when building locally the rustc version is always the same because `omit-git-hash` is on by default. That meant that we depended only on 1 and 3, and we are not very good about bumping METADATA_VERSION (it's currently at 7) so in practice we were only depending on 3. `CrateRoot` is a very large struct and changes somewhat regularly, so this led to a steady stream of crashes from trying to load it. Change the logic to add an intermediate step between 2 and 3: introduce a new `CrateHeader` struct that contains only the minimum info needed to decide whether the crate should be loaded or not. That avoids having to load all of `CrateRoot`, which in practice means we should crash much less often. Note that this works because the SVH should be different between any two dependencies, even if the compiler has changed, because we use `-Zbinary-dep-depinfo` in bootstrap. See rust-lang/rust#111329 (comment) for more details about how the original crash happened.
jyn514
added
beta-nominated
rustbot
removed
the
beta-nominated
…-Simulacrum Revert "Enable incremental independent of stage" This reverts commit 827f656. Incremental is not sound to use across stages. Arbitrary changes to the compiler can invalidate the incremental cache - even changes to normal queries, not incremental itself! - and we do not currently enable `incremental-verify-ich` in bootstrap. Since 2018, we highly recommend and nudge users towards stage 1 builds instead of stage 2, and using `keep-stage` for anything other than libstd is very rare. I don't think the risk of unsoundness is worth the very minor speedup when building libstd. Disable incremental to avoid spurious panics and miscompilations when building with the stage 1 and 2 sysroot. Combined with rust-lang#111329, this should fix rust-lang#76720. r? `@Mark-Simulacrum`
Load only the crate header for `locator::crate_matches` Previously, we used the following info to determine whether to load the crate: 1. The METADATA_HEADER, which includes a METADATA_VERSION constant 2. The embedded rustc version 3. Various metadata in the `CrateRoot`, including the SVH This worked ok most of the time. Unfortunately, when building locally the rustc version is always the same because `omit-git-hash` is on by default. That meant that we depended only on 1 and 3, and we are not very good about bumping METADATA_VERSION (it's currently at 7) so in practice we were only depending on 3. `CrateRoot` is a very large struct and changes somewhat regularly, so this led to a steady stream of crashes from trying to load it. Change the logic to add an intermediate step between 2 and 3: introduce a new `CrateHeader` struct that contains only the minimum info needed to decide whether the crate should be loaded or not. That avoids having to load all of `CrateRoot`, which in practice means we should crash much less often. Note that this works because the SVH should be different between any two dependencies, even if the compiler has changed, because we use `-Zbinary-dep-depinfo` in bootstrap. See rust-lang/rust#111329 (comment) for more details about how the original crash happened.
Load only the crate header for `locator::crate_matches` Previously, we used the following info to determine whether to load the crate: 1. The METADATA_HEADER, which includes a METADATA_VERSION constant 2. The embedded rustc version 3. Various metadata in the `CrateRoot`, including the SVH This worked ok most of the time. Unfortunately, when building locally the rustc version is always the same because `omit-git-hash` is on by default. That meant that we depended only on 1 and 3, and we are not very good about bumping METADATA_VERSION (it's currently at 7) so in practice we were only depending on 3. `CrateRoot` is a very large struct and changes somewhat regularly, so this led to a steady stream of crashes from trying to load it. Change the logic to add an intermediate step between 2 and 3: introduce a new `CrateHeader` struct that contains only the minimum info needed to decide whether the crate should be loaded or not. That avoids having to load all of `CrateRoot`, which in practice means we should crash much less often. Note that this works because the SVH should be different between any two dependencies, even if the compiler has changed, because we use `-Zbinary-dep-depinfo` in bootstrap. See rust-lang/rust#111329 (comment) for more details about how the original crash happened.
Previously, we used the following info to determine whether to load the crate:
CrateRoot, including the SVHThis worked ok most of the time. Unfortunately, when building locally the rustc version is always
the same because
omit-git-hashis on by default. That meant that we depended only on 1 and 3, andwe are not very good about bumping METADATA_VERSION (it's currently at 7) so in practice we were
only depending on 3.
CrateRootis a very large struct and changes somewhat regularly, so this ledto a steady stream of crashes from trying to load it.
Change the logic to add an intermediate step between 2 and 3: introduce a new
CrateHeaderstructthat contains only the minimum info needed to decide whether the crate should be loaded or not. That
avoids having to load all of
CrateRoot, which in practice means we should crash much less often.Note that this works because the SVH should be different between any two dependencies, even if the
compiler has changed, because we use
-Zbinary-dep-depinfoin bootstrap. See#111329 (comment) for more details about how the
original crash happened.
Helps with #76720.