Hashed dependencies of metadata into the metadata of a lib #4469

nipunn1313 · 2017-09-05T00:18:12Z

This fixes one part of #3620. To my understanding, the more fundamental fix is more challenging

rust-highfive · 2017-09-05T00:18:16Z

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

nipunn1313 · 2017-09-05T00:21:16Z

src/cargo/ops/cargo_rustc/context.rs

+            dep_metadatas.sort();
+            for metadata in dep_metadatas {
+                metadata.hash(&mut hasher);
+            }


is it cleaner to just do dep_metadatas.hash(&mut hasher) and rely on Vec's impl of Hash?

As a follow up, would it be nicer to use a sorted datastructure (like BTreeSet) instead of calling .sort() on a Vec? I'm certain it doesn't matter from a perf perspective, so whatever the team likes stylistically is fine to me.

nipunn1313 · 2017-09-05T00:22:11Z

tests/workspaces.rs

+
+
+    // Build caller1. should build the dep library. Because the features
+    // are different than the full workspace, it rebuilds.


It would be beautiful if building the entire workspace simply covered this, but it doesn't as is. According to #3620, this is difficult to fix? I couldn't find an easy way.

I just left this comment which I think is related to this. I do believe it'll be a relatively invasive fix to fix this.

nipunn1313 · 2017-09-05T03:33:39Z

This got a bit nastier after getting the doctests to pass because the fn dep_targets() produces different targets for doctests vs regular tests (reasonably so), but the target_metadata is expected to be the same.

Here, I pull the central code inside dep_targets sans filtering up to the metadata calculation layer, and things worked out. There's probably a way to factor this cleaner, but running through CI / requesting feedback before embarking on that.

matklad · 2017-09-05T05:47:29Z

Huh, oddly enough I've hit this issue myself a couple of days ago: #4463.

However I feel that the problem here is deeper then just spurious rebuilds, because you actually get different artifacts depending on how you build your workspace(see #4463), and this seems pretty bad to me.

@alexcrichton , if workspaces share the same dep graph, perhaps we should always activate the union of all features of the workspace? Currently for features we loop over requested summaries, but it shouldn't be that difficult to loop over all of them?

@nipunn1313 I think that if you add 'cargo test --all --no-run' before the crate loop in your CI script, you won't get rebuilds.

nipunn1313 · 2017-09-05T07:41:54Z

@matklad unfortunately running cargo test --all --no-run is insufficient. The test I added in this diff actually highlights the issue (it runs cargo build on entire workspace, and then in the sub crates). The test fails on master, but is ok with my diff.

I also thought about having cargo build within a crate activating the union of all features in the workspace, but that might increase the startup time for cargo build in large workspaces? It's not too bad, but it's on the order of 20 seconds for us from the root, but near instantaneous within individual crates. We have 70 crates in our workspace with 207 vendored deps (cargo-vendor style)

matklad · 2017-09-05T07:46:19Z

it runs cargo build on entire workspace, and then in the sub crates

cargo build won't build the whole workspace, cargo bulid --all will, so I still think that cargo test --all --no-run should help here :)

matklad · 2017-09-05T07:49:45Z

I still think that cargo test --all --no-run should help here :)

Huh, I am 100% wrong, sorry for the noise then :)

matklad · 2017-09-05T08:02:33Z

that might increase the startup time for cargo build in large workspaces?

I think in theory this should not be the case, because the workspace shares the dependency graph and the lockfile anyway, so even if you compile a single package, the whole workspace needs to be loaded. And that makes me think that the huge difference in start up time between single crate and whole workspace you are observing probably indicates some issue in Cargo.

However I would say that the primary problem here is correctness (producing different artifacts for foo for cargo bulid --p foo versus cargo build --all), and that we probably should fix it first, and then try to regain lost ground in terms of performance, if any. For example, I can imaging caching the result of feature selection just like another build artifact.

alexcrichton · 2017-09-05T15:02:08Z

Thanks for the PR @nipunn1313! I've long wanted to implement this!

@matklad I think we'll want this solution no matter what for a number of reasons. Let's say you're working on just one crate and you do:

cargo build 
# edit files ...
cargo build --features a
# edit files ...
cargo build

I'd personally expect the third build (second usage fo cargo build --features a) to essentially do an incremental compilation based on what's edited. What happens today, though, is that this could trigger a full compilation starting with some super deep dependency working its way back up the tree. The solution here, I believe, is to separately cache artifacts based on feature sets. That is, the cargo build --features a will recompile everything it needs to, but all compiled crates will be cached in different locations based feature sets activated to ensure that we don't oscillate on what's being cached.

This has come up a good number of times in a whole slew of situations! Now the solution a lot of the time is to stop oscillating on features and instead just unify what's used everywhere, but that's not always possible. In any case though I think this boils down to "not a workspace problem" and instead a "how Cargo caches dependencies" problem. (in that workspaces aren't required to reproduce this issue, they just make it worse sometimes)

alexcrichton · 2017-09-05T15:03:46Z

src/cargo/ops/cargo_rustc/context.rs

+        // Mix in the target-metadata of all the dependencies of this target
+        if let Ok(deps) = self.used_deps(unit) {
+            let mut deps_metadata = deps.into_iter().map(|dep_unit| {
+                self.target_metadata(&dep_unit)


So runtime-wise we've had a lot of issues in the past with this sort of recursive calculation causing an exponential amount of code to run instead of linear (wrt the dependency graph). For example here I think that if we call target_metadata for all targets this goes from linear (currently) to exponential (after this PR), right?

Perhaps we can introduce a cache for target_metadata? (that's what we do for everything else!)

This tends to not show up much in small crate graphs but projects like Servo (and in theory Dropbox w/ 200+ crates) may end up showing this pretty badly.

Yeah. That sounds like a great solution. Envisioning a big hashmap from Unit -> Metadata in the ctx? We could probably even precalculate it if we walk the units in dependency order.

Sounds great! I'm fine with doing the caching lazily or doing it all at once when we walk in dependency order. I know we have a few "prepopulate passes" at the beginning to walk the graph, but I sort of forget what they're doing. Basically whatever's easiest is fine by me

alexcrichton · 2017-09-05T15:04:47Z

src/cargo/ops/cargo_rustc/context.rs

-            return self.doc_deps(unit);
-        }
-
+    fn used_deps(&self, unit: &Unit<'a>) -> CargoResult<Vec<Unit<'a>>> {


This seems like a somewhat unfortunate addition in the slew of already-too-many ways to calculate dependencies :(

I didn't quite follow your explanation earlier, mind detailing a bit more what was going on?

Yeah I agree. At least this one is private only. The issue here is the fork on
if unit.profile.run_custom_build and if unit.profile.doc && !unit.profile.test

Specifically, OUT_DIR was set incorrectly in the build phase of doctests because doctests had a different dependency tree, but OUT_DIR was expected to be the same.

Hm I'm still not 100% following... In any case, I'll check this out locally and poke around.

Yeah. Easier if you poke around. I'll give it another shot though.

Without this refactor:

When you compile the build.rs script for a doctest vs compiling the build.rs script for a regular test, it ends up with different metadata. This causes OUT_DIR to get set to a different (nonexistent) directory during doctests. It shows up as a test failure.

https://travis-ci.org/rust-lang/cargo/jobs/271875681

With this refactor

Doctests and regular tests have the same used_deps despite having different dep_targets.

Overall, it definitely does feel like some elements are repeated here and architecturally there is some unnecessary complexity, but I don't understand it well enough right now to find a way out. I think we need one function that returns deps for the package vs deps for the unit of build?

Ah ok that sounds very surprising! The build script should be constant here and shouldn't change metadata, but let me poke around to see if I can't find where the difference is arising from

alexcrichton · 2017-09-05T15:07:09Z

@matklad

@alexcrichton , if workspaces share the same dep graph, perhaps we should always activate the union of all features of the workspace?

I don't think this'd be too hard to implement, but I'm not sure if this is what we'd want implemented per se. If one target of a workspace doesn't want a particular feature activated, wouldn't it be surprising if some other target present in a workspace far away activated the feature?

@nipunn1313

It's not too bad, but it's on the order of 20 seconds for us from the root, but near instantaneous within individual crates

20 seconds in Cargo definitely sounds like a bug to me! I'd love to help investigate this and speed that up if it's a problem, but we can probably take that off this PR :)

matklad · 2017-09-05T15:11:20Z

@alexcrichton yeah, totally agree that the fix here is needed in general!

nipunn1313 · 2017-09-05T15:34:48Z

@alexcrichton We actually do already cache the target separately if the features change. The issue is that we cache the target the same if the features of a dep change (see discussion here #3620). It's a bit more subtle, but still in the same realm as the issue you're describing.

Eg
(x -> y). Features for which x calls y change.
y compiles to a different hash
x compiles to the same hash but links against the new y

Copying my eg from #3620 where x=itertools, y=either

Running `rustc --crate-name itertools itertools-0.6.2/src/lib.rs
--crate-type lib --emit=dep-info,link -C debuginfo=2
-C metadata=4ed3e3cf3bc8df3d -C extra-filename=-4ed3e3cf3bc8df3d
--out-dir target/debug/deps
-L dependency=target/debug/deps
--extern either=target/debug/deps/libeither-f93178e8a5af0b1d.rlib
--cap-lints allow`

vs

Running `rustc --crate-name itertools itertools-0.6.2/src/lib.rs
--crate-type lib --emit=dep-info,link -C debuginfo=2
-C metadata=4ed3e3cf3bc8df3d -C extra-filename=-4ed3e3cf3bc8df3d
--out-dir target/debug/deps
-L dependency=target/debug/deps 
--extern either=target/debug/deps/libeither-4dcab0f19fb09534.rlib
--cap-lints allow`

FWIW, I think #3620 and #4463 may be duplicates. Lots of good discussion in both.

nipunn1313 · 2017-09-05T15:35:35Z

src/cargo/ops/cargo_rustc/context.rs

@@ -483,6 +483,15 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
        // when changing feature sets each lib is separately cached.
        self.resolve.features_sorted(unit.pkg.package_id()).hash(&mut hasher);


We've always mixed in features for the package itself. Just not for the deps. See this line

nipunn1313

I'll work on the cache to replace the recursive call.

nipunn1313 · 2017-09-05T15:36:46Z

src/cargo/ops/cargo_rustc/context.rs

+        // Mix in the target-metadata of all the dependencies of this target
+        if let Ok(deps) = self.used_deps(unit) {
+            let mut deps_metadata = deps.into_iter().map(|dep_unit| {
+                self.target_metadata(&dep_unit)


Yeah. That sounds like a great solution. Envisioning a big hashmap from Unit -> Metadata in the ctx? We could probably even precalculate it if we walk the units in dependency order.

nipunn1313 · 2017-09-05T15:39:49Z

src/cargo/ops/cargo_rustc/context.rs

-            return self.doc_deps(unit);
-        }
-
+    fn used_deps(&self, unit: &Unit<'a>) -> CargoResult<Vec<Unit<'a>>> {


Yeah I agree. At least this one is private only. The issue here is the fork on
if unit.profile.run_custom_build and if unit.profile.doc && !unit.profile.test

Specifically, OUT_DIR was set incorrectly in the build phase of doctests because doctests had a different dependency tree, but OUT_DIR was expected to be the same.

alexcrichton · 2017-09-05T15:44:03Z

@nipunn1313 ah yeah I believe you and I are worried about the same case! I forgot long ago when you added "hash the feature selection into the metadata" to also account for the transitive case :(

nipunn1313 · 2017-09-05T16:47:44Z

Cool. Just worked out the cache. Realized as I was writing it that I worked on one of the other caches (for target_filenames) last year. Had forgotten heh.

alexcrichton · 2017-09-05T21:57:49Z

Ok so one (existing) bug I've found is that the target_metadata for a build script changes over time. Basically when you have internal mutability then things go wrong.

I've fixed that with this diff. There's one failure, however, remaining with the doctest issue like you were mentioning, looking into that now.

alexcrichton · 2017-09-05T22:06:17Z

Ok turns out the next bug is actually the same. After we've compiled everything a Compilation structure is built up which saves off OUT_DIR, but at that point the build_state is all filled in so there's a different set of listed dependencies for build scripts than before, causing the OUT_DIR that rustdoc --test uses to be different from the main compilation.

I fixed that test in specific by moving this line below this line, but that unfortunately breaks the output_depinfo line just above it.

I think the tl;dr; here is that the internal mutability causing a difference in dep_targets is the "root of all evil" here. Perhaps the Context build up, very early on, a map of what units are overridden and then deterministically skip or not skip them all in invocations of dep_targets? I think the custom_build::build_map function is likely the best place to build such a map (and feel free to shove it anywhere on Context)

Previously it depended on dynamic state that was calculated throughout a compilation which ended up causing different fingerprints showing up in a few locations, so this makes the invocation deterministic throughout `cargo_rustc`.

alexcrichton · 2017-09-09T20:58:20Z

@bors: r+

Alright I pushed up some small fixes, let's see what @bors thinks

bors · 2017-09-09T20:58:21Z

📌 Commit f90d21f has been approved by alexcrichton

Hashed dependencies of metadata into the metadata of a lib This fixes one part of #3620. To my understanding, the more fundamental fix is more challenging

bors · 2017-09-09T21:39:50Z

⌛ Testing commit f90d21f with merge 921c4a5...

bors · 2017-09-09T23:20:29Z

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

nipunn1313 · 2017-09-10T02:45:50Z

Nice find Alex!
Thanks for patching it up and pushing it through.

rust-highfive assigned matklad Sep 5, 2017

nipunn1313 commented Sep 5, 2017

View reviewed changes

nipunn1313 force-pushed the workspace_features branch from 679639a to cbae325 Compare September 5, 2017 08:48

alexcrichton reviewed Sep 5, 2017

View reviewed changes

matklad mentioned this pull request Sep 5, 2017

Feature selection in workspace depends on the set of packages compiled #4463

Open

nipunn1313 commented Sep 5, 2017

View reviewed changes

nipunn1313 and others added 6 commits September 9, 2017 13:46

Hashed dependencies of metadata into the metadata of a lib

46a5fe9

get doctests to pass

69e0d9f

Refactor to share code from dep_targets. Fixes the plat-specific case

7336e32

Cleanup debug statements

d2ca374

DP Cache target_metadata. Update all the lifetimes

67b3b44

Make dep_targets consistent throughout compilation

f90d21f

Previously it depended on dynamic state that was calculated throughout a compilation which ended up causing different fingerprints showing up in a few locations, so this makes the invocation deterministic throughout `cargo_rustc`.

alexcrichton force-pushed the workspace_features branch from 52e8a72 to f90d21f Compare September 9, 2017 20:56

bors merged commit f90d21f into rust-lang:master Sep 9, 2017

bors mentioned this pull request Sep 9, 2017

Handle overlapping members and exclude keys #4297

Closed

alexcrichton mentioned this pull request Sep 26, 2017

fingerprint error causing rebuild on feature flag changes #3923

Closed

nipunn1313 deleted the workspace_features branch August 6, 2021 18:03

ehuss added this to the 1.22.0 milestone Feb 6, 2022



		// Build caller1. should build the dep library. Because the features
		// are different than the full workspace, it rebuilds.

		@@ -483,6 +483,15 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
		// when changing feature sets each lib is separately cached.
		self.resolve.features_sorted(unit.pkg.package_id()).hash(&mut hasher);

Hashed dependencies of metadata into the metadata of a lib #4469

Hashed dependencies of metadata into the metadata of a lib #4469

Conversation

nipunn1313 commented Sep 5, 2017

rust-highfive commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nipunn1313 commented Sep 5, 2017

matklad commented Sep 5, 2017

nipunn1313 commented Sep 5, 2017

matklad commented Sep 5, 2017

matklad commented Sep 5, 2017

matklad commented Sep 5, 2017

alexcrichton commented Sep 5, 2017

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

alexcrichton commented Sep 5, 2017

matklad commented Sep 5, 2017

nipunn1313 commented Sep 5, 2017

Choose a reason for hiding this comment

nipunn1313 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Sep 5, 2017

nipunn1313 commented Sep 5, 2017

alexcrichton commented Sep 5, 2017 • edited Loading

alexcrichton commented Sep 5, 2017

alexcrichton commented Sep 9, 2017

bors commented Sep 9, 2017

bors commented Sep 9, 2017

bors commented Sep 9, 2017

nipunn1313 commented Sep 10, 2017

alexcrichton commented Sep 5, 2017 •

edited

Loading