Refactor BuildContext

[ehuss]

This restructures the "front end" of the compile process so that the UnitGraph can be accessed by API users. Essentially, the BuildContext contains the result of generating the UnitGraph, and other bits of information collected along the way. This logically separates the build process into two phases: (1) generate the UnitGraph and BuildContext and (2) pass the BuildContext to Context which performs the actual compilation.

The main challenge here is dealing with the references and lifetimes. The old code kept a bunch of things on the stack with various layers of references. Beside reorganizing things, the big change is to wrap Package and Target in Rc. This still requires the UnitInterner to be passed in and kept alive. It is possible to avoid that by placing all Units in Rc, but that had a roughly 5% performance hit (on fresh builds) because Units are very optimized to be used as hashable keys, and Rc loses those optimizations.

[rust-highfive]

r? @alexcrichton

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

[ehuss]

I don't know if we should actually merge this. I made this a while ago, but I no longer need it. But Alex said I should post it anyways, so here it is.

Some parts I think are a little cleaner. But this is a large PR, touching lots of stuff. It is separated in 2 commits.

[djc]

FWIW (as someone who did a bunch of refactoring in this area before), it looks pretty nice to me.

[alexcrichton]

@ehuss can you remind me the benchmark you're using? I'm curious to test out a few things and see what the perf looks like

[ehuss]

Any project with a lot of crates should work. Cargo itself is a smallish example, servo or something else big works too. The playground's base project is also good as it gets 100 popular crates or about 300 deps total (generated by this script). I also have something similar in cargo-prefetch called make-top which will list the top 1000 crates (pass it a path to a crates.io-index clone). It's a little finicky because of links collisions, so it takes some manual work.

I also use an artificial workspace (though it isn't great because it doesn't have dependencies):

#!/bin/sh

set -e

for (( i=0; i<$1; ++i))
do
    cargo new --lib dep${i}
done

cat << EOF > Cargo.toml
[workspace]
members = ["dep*"]
EOF

Run cargo check once to build everything, then hyperfine "cargo check" (link).

[alexcrichton]

Ok thanks!

So here's some things I've tried:

• Stylistically I'd prefer to avoid Rc<Package> and instead use Package { inner: Rc<Internals> } which is how Dependency and Summary work today -- alexcrichton@3476f53
• Similarly I'd prefer to do the same for Target, but Target actually needs to move across threads so it'd be Target { inner: Arc<Internals> }. I did it in alexcrichton@28cf4c2 but forgot to separate out the changes.
• Finally I tried to replicate making Unit { inner: Rc<Internals> } to make it a completely owned value that needs no lifetime parameter (making the UnitInterner's lifetime irrelevant. I was able to also see a small perf hit, but it feels like it's very negligible. If Rc refcount management is a perf hit then I think we need to figure out how to architect things to clone less rather than avoid the Rc in the first place. For example we could try switching to struct UnitId(usize) and then you just always need a Context to learn about the UnitId itself (and store everything in a Vec)

Overall I feel like we can take this a step further (afterwards if you'd prefer) to make Unit an owned value. Did you have a similar implementation though and conclude that the perf hit was too much?

[ehuss]

Those changes sound fine. The Arc is a little unfortunate, but if it doesn't have much performance impact, seems good!

I couldn't tell, but it looks like you didn't commit your final experiment removing the Unit lifetime.

Here is my old branch where I wrapped Unit in Rc: ehuss@c053288. Doing some more testing, I guess in absolute terms the performance hit isn't too bad. For me, on the playground project, a fresh build goes from 250ms to 270ms. I guess most people won't notice 20ms, although this is a fast-ish machine.

If you want, you can push changes to this branch/PR.

[alexcrichton]

Here is my old branch where I wrapped Unit in Rc

ah ok this explains a bit to me. The Hash for Unit subtly changed in your PR where previously it just hashes the pointer to the unit (very fast) but with your PR it's actually hashing the contents of the unit (since Hash for Rc<T> forwards to T). In my changes I left Unit with an Rc internals so the hash should still be just a pointer. In light of that yeah I'll push these changes up and try to clean up a bit too.

I didn't go through the actual removal of the lifetime since it seemed like it would be hard, but I'll go ahead and do so now :)

[alexcrichton]

Ok I've pushed up the commits I had, in addition to the removal of a lifetime from Unit. Holy cow that was a lot of lifetimes removed.

[alexcrichton]

I suppose in terms of next steps, @ehuss want to review what I've done and then we can land this and continue to make progress in-tree?

[bors]

☔ The latest upstream changes (presumably #8087) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

OK, looks good! I did some testing and didn't see any performance differences. 🤞

@bors r+

[bors]

📌 Commit 57ad41e1a83da3aafe42895cc29a69414f863539 has been approved by ehuss

[bors]

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

1. git checkout bcx-units (switch to your branch)
2. git fetch upstream master (retrieve the latest master)
3. git rebase upstream/master -p (rebase on top of it)
4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
5. git push self bcx-units --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging src/cargo/core/manifest.rs
Auto-merging src/cargo/core/compiler/mod.rs
Auto-merging src/cargo/core/compiler/context/compilation_files.rs
CONFLICT (content): Merge conflict in src/cargo/core/compiler/context/compilation_files.rs
Automatic merge failed; fix conflicts and then commit the result.

[bors]

☔ The latest upstream changes (presumably #8135) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

@bors r=alexcrichton,ehuss

[bors]

📌 Commit df5cb70 has been approved by alexcrichton,ehuss

[bors]

⌛ Testing commit df5cb70 with merge 3dcfdef...

[bors]

☀️ Test successful - checks-azure
Approved by: alexcrichton,ehuss
Pushing 3dcfdef to master...