Utilize dep-info for all dependencies

[Mark-Simulacrum]

Now that sysroot dependencies are encoded, we want to know about them
regardless of whether the crate itself is unlikely to change contents.

r? @alexcrichton

[Mark-Simulacrum]

See #7030 (comment) for context here; we shouldn't land it before that PR but the analysis of whether this is the right change or something more limited is necessary can be done in parallel I think.

[ehuss]

Can you also delete the first two sentences in the "Special considerations" section of the documentation at the top of the file (the ones that talk about registry dependencies not using mtimes)?

[alexcrichton]

Ok I think this probably isn't going to have any adverse effects really, it does look very local in the code about simply using the dep-info file in more locations.

I am worried, however, as to the performance impact of this change. Especially on slower filesystems this is likely to make Cargo issue an order of magnitude more stat calls than before. Previously you're only stat'ing files that you are locally modifying, but all your upstream dependencies (on the order of hundreds) are largely ignored. Now, however, every file that could be part of the build is being stat'd.

This is probably fine for rustc where the vast majority of files are all local anyway, but I'd be worried about builds like Servo or Cargo itself where the vast majority of files come from crates.io or git dependencies. Would it be possible to do some analysis to see try to evaluate the performance impact and or stat-system-call impact this PR has?

[Mark-Simulacrum]

Building cargo itself, on macOS (I won't have access to a linux machine until approximately Wednesday next week). Looks like this is behaving relatively equally performance wise (and only ~500 more stat calls than before, ish on a no-op build of Cargo). If there's some other scenario that'd be good to test then I'm happy to do so, though I might wait till I have access to linux as I'm more familiar with the tooling there.

master:

cargo check (full build)
Time (mean ± σ): 73.439 s ± 5.743 s [User: 258.451 s, System: 22.710 s]
Range (min … max): 63.240 s … 78.199 s 10 runs

full build syscall stats:

            poll               1028
         fstat64               1080
           ioctl               1993
            open               2036
          stat64               2134
  close_nocancel               3186
            read               4001
           write               5327

cargo check (no-op build)
Time (mean ± σ): 214.1 ms ± 1.5 ms [User: 133.3 ms, System: 68.2 ms]
Range (min … max): 212.3 ms … 216.8 ms 13 runs

no-op build stats:

         fstat64                755
            open                777
  close_nocancel                871
            read               1264
          stat64               1581

This PR:

cargo check (full build)
Time (mean ± σ): 69.438 s ± 3.746 s [User: 242.610 s, System: 21.199 s]
Range (min … max): 62.643 s … 72.185 s 10 runs

full build stats:

            poll               1016
            open               1858
          stat64               1909
           ioctl               2041
  close_nocancel               2696
            read               3063
           write               5539

cargo check (no-op build)
Time (mean ± σ): 221.4 ms ± 3.2 ms [User: 136.2 ms, System: 73.4 ms]
Range (min … max): 217.1 ms … 229.5 ms 12 runs

no-op build stats:

   open_nocancel                491
         fstat64                824
            open                846
  close_nocancel                937
            read               1483
          stat64               2378

[Mark-Simulacrum]

I haven't looked into the test failures deeply but they seem rather odd, perhaps exposing prior bugs? Not sure.

[ehuss]

This also causes some sporadic failures on HFS. I'd like to see that resolved before merging this. Some that seem to fail are:

cache_messages::clears_cache_after_fix
freshness::bust_patched_dep
freshness::fingerprint_cleaner_does_not_rebuild
freshness::script_fails_stay_dirty
freshness::simple_deps_cleaner_does_not_rebuild

I can try to help look at them soonish.

[alexcrichton]

Hm ok, I'm not necessarily concerned too much about a particular project but moreso how this scales. If we're required to always stat everything that your project depends on then that could get quite unwieldy for larger projects while being invisible effectively in smaller projects (like Cargo)

[Mark-Simulacrum]

Even on a project of Servo's size it's only ~5000 additional syscalls on a no-op debug build (out of 60,000 syscalls total). That leads me to believe that there's not much reason to be concerned here -- the time impact and scale of this change is fairly minimal. It's definitely not an order of magnitude increase.

Is there some concrete number/project I can gather? I do think this is adding work but for the most part not that much -- the number of dependencies and as such files that need to be stat'd is I'd expect smaller by about an order of magnitude than the number of local source files that we need to stat anyway, so this feels like not too big a concern to me.

[Mark-Simulacrum]

I can't reproduce the failures on HFS with CI set in the environment I think, after running cargo test --release freshness a couple times (do I need hundreds?). @ehuss could you perhaps investigate since you seem to be able to reproduce?

[ehuss]

Oh, crap, I'm sorry. I ran without the CI environment variable, I forgot. Ignore my note about it, then.

And I've been intending to get back to looking at this, I just haven't made the time.

[ehuss]

I've been digging into this and rust-lang/rust#61727 for a while today, but I've run out of time and energy.

There's a bug with #7030 in that it doesn't handle host-vs-target directories correctly. I hacked together a fix, but it ended up changing quite a lot. The target directory handling code is too brittle and the naming conventions are a bit confusing. I'm not sure what ultimately will be the best way to fix it. I'm tempted to do something more radical to make it easier to work with, but I'm not sure.

If we want to mitigate the concern about performance, the dep-info parser could skip package-relative files for non-path dependencies. Perhaps something like this:

diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs
index 1804be9a..9eabb106 100644
--- a/src/cargo/core/compiler/fingerprint.rs
+++ b/src/cargo/core/compiler/fingerprint.rs
@@ -1415,11 +1415,11 @@ pub fn parse_dep_info(
             }
         })
         .collect::<Result<Vec<_>, _>>()?;
-    if paths.is_empty() {
-        Ok(None)
-    } else {
-        Ok(Some(paths))
-    }
+    // if paths.is_empty() {
+    //     Ok(None)
+    // } else {
+    Ok(Some(paths))
+    // }
 }

 fn pkg_fingerprint(bcx: &BuildContext<'_, '_>, pkg: &Package) -> CargoResult<String> {
@@ -1541,6 +1541,7 @@ pub fn translate_dep_info(
     rustc_cwd: &Path,
     pkg_root: &Path,
     target_root: &Path,
+    allow_package: bool,
 ) -> CargoResult<()> {
     let target = parse_rustc_dep_info(rustc_dep_info)?;
     let deps = &target
@@ -1552,6 +1553,10 @@ pub fn translate_dep_info(
     for file in deps {
         let file = rustc_cwd.join(file);
         let (ty, path) = if let Ok(stripped) = file.strip_prefix(pkg_root) {
+            if !allow_package {
+                // eprintln!("skipping {:?} {:?}", pkg_root, stripped);
+                continue;
+            }
             (DepInfoPathType::PackageRootRelative, stripped)
         } else if let Ok(stripped) = file.strip_prefix(target_root) {
             (DepInfoPathType::TargetRootRelative, stripped)
diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs
index 628ac8bb..9d85874a 100644
--- a/src/cargo/core/compiler/mod.rs
+++ b/src/cargo/core/compiler/mod.rs
@@ -321,6 +321,7 @@ fn rustc<'a, 'cfg>(
                 &cwd,
                 &pkg_root,
                 &root_output,
+                current_id.source_id().is_path()
             )
             .chain_err(|| {
                 internal(format!(

I don't know why empty dep-info files are treated as "missing". It was changed in #4788, but there aren't any notes as to why.

It would still end up stating the sysroot files a lot, but I would guess a cached stat should be pretty fast. That could be further mitigated by caching some of those stat calls.

[Mark-Simulacrum]

There's a bug with #7030 in that it doesn't handle host-vs-target directories correctly.

It's not clear to me what you mean by this -- do you have a test case? That code (IIRC) didn't actually root paths at the target directory, so I'm somewhat surprised that it is buggy in that way. The target_root in code is I believe the workspace root, though the naming could be better.

[ehuss]

do you have a test case?

Using it in rust-lang/rust with rust-lang/rust#61727. Or, make a workspace with a member that has a dependency on serde_derive and uses it, and build with --target $host. Building again will cause a rebuild instead of being fresh. Build caching won't work because the path in the dep-info file to libserde_derive.dylib is wrong. It gets stored in the dep-info file as deps/libserde_derive-86a83b67ebaa0bbd.dylib, but that is the wrong relative directory for host artifacts when using --target.

I also think there is some issue with it being in a directory inside a workspace vs not (since this only happens when the project is a subdirectory in a workspace).

I think the crux of the problem is target_root, a poorly named function. It returns 3 very different paths.

My hack involved changing it to return Workspace::target_dir instead so that all paths are relative to the very root of the target directory. This requires a number of changes so the rest of the code works, particularly build scripts. I think that should be the right approach, as it simplifies things. However, it still seems quite brittle.

Let me know what you think. I haven't quite finished reviewing all the different relative paths. I probably won't have much time to work on it this weekend.

Here is a diff of my hack. IIRC, this passes all tests (after removing the debugging prints), and fixes the rebuild issue. I don't think it should be used as-is, though. I know Alex doesn't like the use of into_path_unlocked 😜 . My feeling is that it would be nice to centralize all target directory management in one place. It's currently somewhat scattered. (This is also related to my target dir rework such as #6668, but has a long way to go.)

[Mark-Simulacrum]

I also think there is some issue with it being in a directory inside a workspace vs not (since this only happens when the project is a subdirectory in a workspace).

I think this bit is what that PR tries to fix, but I didn't consider that --target would (apparently) change things enough to cause problems.

I'm obviously less knowledgeable in this area -- and not sure what the next step should be. If you have suggestions I'm happy to implement them or try to make progress; otherwise I consider this somewhat blocked on a "what to do next." I can attempt to fix the --target handling, but from what you've said a targeted fix is unlikely.

[Mark-Simulacrum]

Closing in favor of b1b9b79.