Rustc cache #5359

matklad · 2018-04-14T10:46:00Z

This implements rustc caching, to speed-up no-op builds.

The cache is per-project, and stored in target directory. To implement this, I had to move rustc from Config down to BuildConfig.

closes #5315

We want to create `rustc` only when we already have workspace, so that we could use the `target` dir to store cached info about the compiler.

rust-highfive · 2018-04-14T10:46:10Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

matklad · 2018-04-14T10:49:52Z

src/cargo/util/rustc.rs

+            paths::mtime(&rustup_rustc)?.hash(&mut hasher);
+        }
+        _ => (),
+    }


This match requires some scrutiny during review, because it is not tested, but kinda important :-)

We can't just look at RUSTUP_TOOLCHAIN, because the toolchain name does not change when you do rustup update, it is just stable-x86_64-unknown-linux-gnu. So we actually have to look inside the toolchain and mtime the real rustc binary.

This still has a failure mode unfortunately :( If the cargo is not rustup, but rustc is rustup, then these env variables won't be set, and we won't be able to detect compieler changes.

One possible mitigation here I think would be to look at the result of the resolved executable. If rustc resolves to $CARGO_HOME/.bin/rustc then I think we can say with a high level of certainty that it's a rustup rustc. We could even say that if we detect a rustup rustc and RUSTUP_HOME and/or RUSTUP_TOOLCHAIN isn't set that we skip this cache entirely.

Yep, implemented that logic!

matklad · 2018-04-14T14:40:12Z

@bors try

bors · 2018-04-14T14:40:20Z

⌛ Trying commit 42f347a with merge 88ab8ed...

Rustc cache This implements rustc caching, to speed-up no-op builds. The cache is per-project, and stored in `target` directory. To implement this, I had to move `rustc` from `Config` down to `BuildConfig`. closes #5315

bors · 2018-04-14T14:44:11Z

💔 Test failed - status-appveyor

djc

I don't quite get what the earlier refactorings are trying to achieve. However, I highly appreciate that we should probably cache the output of the rustc invocation. Can't we just hide the cache inside Config::rustc(). Also, should we (first? I don't know) try to combine the calls to rustc for getting the version and the TargetInfo?

djc · 2018-04-15T18:22:28Z

src/cargo/util/config.rs

-                self.maybe_get_tool("rustc_wrapper")?,
-            )
-        })
+    pub fn new_rustc(&self) -> CargoResult<Rustc> {


Why rename this here? Is that just cosmetic? Maybe extract it into a separate commit that explains why this makes sense on its own?

djc · 2018-04-15T18:24:38Z

src/cargo/core/compiler/context/compilation_files.rs

-    if let Ok(rustc) = cx.config.rustc() {
-        rustc.verbose_version.hash(&mut hasher);
-    }
+    cx.build_config.rustc.verbose_version.hash(&mut hasher);


In my perception, it seemed that build_config should probably stay immutable over the lifetime of the Context (and in fact that is enforced, I think, by #5358).

Hm, not sure I understand this comment :) build_config is not mutated here at all?

matklad · 2018-04-15T19:37:02Z

I don't quite get what the earlier refactorings are trying to achieve. However, I highly appreciate that we should probably cache the output of the rustc invocation. Can't we just hide the cache inside Config::rustc(

I’ve initially tried to just add caching to Config.rustc, but ended up moving rustc to BuildConfig first. The reason is that we need to store the cache somewhere, and the logical choice is to store it in the target directory of the project. However, Config does not know the target directory, because it does not know the current project.

djc · 2018-04-15T19:53:26Z

I’ve initially tried to just add caching to Config.rustc, but ended up moving rustc to BuildConfig first. The reason is that we need to store the cache somewhere, and the logical choice is to store it in the target directory of the project. However, Config does not know the target directory, because it does not know the current project.

Yeah, that part actually makes sense to me -- I had been thinking about doing the same. However, it seems you've made the mechanics of doing so more complicated than what seems necessary. I guess you had some other goals at the same time, or maybe this rustc thing is more complicated than what I've understood so far.

matklad · 2018-04-16T11:57:31Z

@bors try

alexcrichton

Looking fantastic to me, thanks so much for doing this @matklad!

One final aspect I'd want to consider as well is the rustbuild integration aspect. I think though that all the logic here is sufficient to detect changes in that situation, and we can always set the env var if need be to disable this caching entirely.

alexcrichton · 2018-04-16T14:01:42Z

src/cargo/util/paths.rs

@@ -129,6 +158,12 @@ pub fn append(path: &Path, contents: &[u8]) -> CargoResult<()> {
    Ok(())
 }

+pub fn mtime(path: &Path) -> CargoResult<FileTime> {
+    let meta =
+        fs::metadata(path).chain_err(|| internal(format!("failed to stat `{}`", path.display())))?;


In general internal hasn't worked out great for Cargo and I've been trying to remove it wherever I see it, so while you're here mind removing this internal?

alexcrichton · 2018-04-16T14:02:48Z

src/cargo/util/rustc.rs

+
+/// It is a well known that `rustc` is not the fastest compiler in the world.
+/// What is less known is that even `rustc --version --verbose` takes about a
+/// hundred milliseconds! Because we need compiler version info even for no-op


Could this cc the Cargo/rustc issues as well?

alexcrichton · 2018-04-16T14:06:18Z

src/cargo/util/rustc.rs

+
+impl Drop for Cache {
+    fn drop(&mut self) {
+        match (&self.cache_location, self.dirty) {


Perhaps this could be started with:

if !self.dirty { return }

Er well I'll leave it up to you, I'm just personally always looking to aggressively reduce indent!

Yeah, it looks much nicer that way, because I don't have to wrangle with not moving out cahce_location as well!

alexcrichton · 2018-04-16T14:07:28Z

src/cargo/util/rustc.rs

+
+    paths::mtime(&path)?.hash(&mut hasher);
+
+    match (env::var("RUSTUP_HOME"), env::var("RUSTUP_TOOLCHAIN")) {


Could you insert a comment here as to what's going on?

alexcrichton · 2018-04-16T14:09:33Z

src/cargo/util/rustc.rs

+            paths::mtime(&rustup_rustc)?.hash(&mut hasher);
+        }
+        _ => (),
+    }


One possible mitigation here I think would be to look at the result of the resolved executable. If rustc resolves to $CARGO_HOME/.bin/rustc then I think we can say with a high level of certainty that it's a rustup rustc. We could even say that if we detect a rustup rustc and RUSTUP_HOME and/or RUSTUP_TOOLCHAIN isn't set that we skip this cache entirely.

alexcrichton · 2018-04-16T14:10:49Z

src/cargo/ops/cargo_compile.rs

@@ -260,7 +260,8 @@ pub fn compile_ws<'a>(
        bail!("jobs must be at least 1")
    }

-    let mut build_config = BuildConfig::new(config, jobs, &target)?;
+    let rustc_info_cache = ws.target_dir().join("rustc_info.json").into_path_unlocked();


Mind throwing a . on the front to hide it away?

matklad · 2018-04-16T14:52:50Z

However, it seems you've made the mechanics of doing so more complicated than what seems necessary.

Quite probably, but, if I did this, it is because of being stupid, and not because of being smart :-) What simplifications would you suggest?

alexcrichton · 2018-04-16T17:23:50Z

@bors: r+

bors · 2018-04-16T17:23:51Z

📌 Commit 76b0a8a has been approved by alexcrichton

bors · 2018-04-16T17:23:59Z

⌛ Testing commit 76b0a8a with merge d0829a2...

Rustc cache This implements rustc caching, to speed-up no-op builds. The cache is per-project, and stored in `target` directory. To implement this, I had to move `rustc` from `Config` down to `BuildConfig`. closes #5315

matklad · 2018-04-16T17:29:22Z

For posterity, a couple of more details:

write to the .rustc_info.json is racy across Cargo processes. This shoudn't be a problem because we always ignore errors in this file and, in practice, it should be mostly read-only because we write cache only if something did change.
it's interesting to compare this with what sccache does. sccache is actually pretty smart: for compiler fingerprint, it hashes the contents of rustc --print sysroot, which looks like a bulletproof way to do compiler fingerprinting. Unfortunately, we can't use this here because it requires to launch compiler once, the very thing we'd love to avoid.

bors · 2018-04-16T17:52:58Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d0829a2 to master...

matklad · 2018-04-16T18:42:35Z

Let's tag it relnotes, because some folks might need to be aware of CARGO_CACHE_RUSTC_INFO, if they use something like rustup, but not rustup, to manage toolchains.

I think this was left behind as part of rust-lang#5359.

Remove some minor, unused code. I think this was left behind as part of #5359.

luser · 2018-07-27T16:00:51Z

it's interesting to compare this with what sccache does. sccache is actually pretty smart: for compiler fingerprint, it hashes the contents of rustc --print sysroot, which looks like a bulletproof way to do compiler fingerprinting. Unfortunately, we can't use this here because it requires to launch compiler once, the very thing we'd love to avoid.

Coming in here late but I ran into this issue while reading old release notes. Note that sccache only uses the hash of the sysroot libs as input to the hash function for compilation caching--it doesn't recalculate that hash every time (that'd be expensive!) it caches that hash internally with the same strategy you're using here (by comparing mtimes). sccache doesn't have the checks for rustup you added, we've got an open issue on handling rustup properly.

matklad added 4 commits April 14, 2018 11:12

Extract mtime calculation

8c5c9e3

Associated Rustc with a build config

f5f23c2

We want to create `rustc` only when we already have workspace, so that we could use the `target` dir to store cached info about the compiler.

Hide BuildConfig.host_triple behind a getter

52c9f02

Speedup no-op builds by caching rustc invocations

6b1dc52

rust-highfive assigned alexcrichton Apr 14, 2018

matklad commented Apr 14, 2018

View reviewed changes

matklad added 2 commits April 14, 2018 14:09

Add env-var to suppress rustc caching

c473479

Correctly resolve executables in paths on windows

42f347a

djc reviewed Apr 15, 2018

View reviewed changes

alexcrichton reviewed Apr 16, 2018

View reviewed changes

matklad added 5 commits April 16, 2018 18:01

Remove internal from error handling

e2f3378

Handle non-rustup cargo + rustup rustc in rustc cache

501823b

Make rustc_info a hidden file

9e5aaf9

Simplify

c0e326d

rustc is rustc.exe on windows

76b0a8a

matklad force-pushed the rustc-cache branch from 7bce5d1 to 76b0a8a Compare April 16, 2018 16:24

bors merged commit 76b0a8a into rust-lang:master Apr 16, 2018

bors mentioned this pull request Apr 16, 2018

Let CompileOptions create a BuildConfig directly #5358

Merged

matklad deleted the rustc-cache branch April 16, 2018 18:41

matklad added the relnotes Release-note worthy label Apr 16, 2018

matklad mentioned this pull request Apr 16, 2018

Make resolution with correct lock file faster #5321

Closed

ehuss added a commit to ehuss/cargo that referenced this pull request Jun 4, 2018

Remove some minor, unused code.

ce07b75

I think this was left behind as part of rust-lang#5359.

ehuss mentioned this pull request Jun 4, 2018

Remove some minor, unused code. #5607

Merged

bors added a commit that referenced this pull request Jun 4, 2018

Auto merge of #5607 - ehuss:config-rustc, r=matklad

d33c65c

Remove some minor, unused code. I think this was left behind as part of #5359.

alexcrichton mentioned this pull request Oct 22, 2021

Don't canonicalize executable path #9991

Merged

ehuss added this to the 1.27.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustc cache #5359

Rustc cache #5359

matklad commented Apr 14, 2018

rust-highfive commented Apr 14, 2018

matklad Apr 14, 2018

alexcrichton Apr 16, 2018

matklad Apr 16, 2018

matklad commented Apr 14, 2018

bors commented Apr 14, 2018

bors commented Apr 14, 2018

djc left a comment

djc Apr 15, 2018

djc Apr 15, 2018

matklad Apr 16, 2018

matklad commented Apr 15, 2018

djc commented Apr 15, 2018 •

edited

Loading

matklad commented Apr 16, 2018

alexcrichton left a comment

alexcrichton Apr 16, 2018

alexcrichton Apr 16, 2018

matklad Apr 16, 2018

alexcrichton Apr 16, 2018

alexcrichton Apr 16, 2018

matklad Apr 16, 2018

alexcrichton Apr 16, 2018

matklad Apr 16, 2018

alexcrichton Apr 16, 2018

alexcrichton Apr 16, 2018

matklad Apr 16, 2018

matklad commented Apr 16, 2018

alexcrichton commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

matklad commented Apr 16, 2018

bors commented Apr 16, 2018

matklad commented Apr 16, 2018

luser commented Jul 27, 2018


		paths::mtime(&path)?.hash(&mut hasher);

		match (env::var("RUSTUP_HOME"), env::var("RUSTUP_TOOLCHAIN")) {

Rustc cache #5359

Rustc cache #5359

Conversation

matklad commented Apr 14, 2018

rust-highfive commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 14, 2018

bors commented Apr 14, 2018

bors commented Apr 14, 2018

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 15, 2018

djc commented Apr 15, 2018 • edited Loading

matklad commented Apr 16, 2018

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Apr 16, 2018

alexcrichton commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

matklad commented Apr 16, 2018

bors commented Apr 16, 2018

matklad commented Apr 16, 2018

luser commented Jul 27, 2018

djc commented Apr 15, 2018 •

edited

Loading