Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add build profile. #6577

Closed
wants to merge 1 commit into from
Closed

Add build profile. #6577

wants to merge 1 commit into from

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 21, 2019

This adds a build profile as discussed at rust-lang/rust#48683. See unstable.md for a brief description.

@ehuss
Copy link
Contributor Author

ehuss commented Jan 21, 2019

I have some concerns about this, so I wanted to open this for discussion.

dev/release switching

With this feature, if a dev build is made, and then a release build is made, all of the build artifacts are unnecessarily compiled again because they are in different directories. Some thoughts to work around this:

  • Use a dedicated build directory. This would make it difficult/impossible to share common dependencies.
  • Combine debug and release directories. This would make it easier to share artifacts. Of course this would probably cause madness for tools that make assumptions about Cargo's internal layout. I'm not really sure why they are separate directories.

Extra artifacts

I'm very worried about this causing shared dependencies to now be built multiple times.

Some options:

  • Set the defaults of build to the same as dev so that shared deps get reused opportunistically. This would not help for release builds.
  • Reuse shared dependencies preferring dev or release artifacts if available (and compatible). However, I think that would be very difficult to implement. The unit dependency builder might need to be a two-pass algorithm, which would complicate it quite a bit.

I've been doing some analysis on crates.io to try to understand the impact.

3187 of 21098 crates have at least one overlapping dependency. gist. crossgen has a whopping 161 crates in common worst case.

I did some tests "without" build profile and "with" build profile with either 12 or 2 concurrency. All times in seconds. These is just a rough idea. I ran each attempt multiple times, but this is particular to my hardware, and running on MacOS.

Package j12 Without j12 With j2 Without j2 With Notes
rand 44.3 40.6 (⨉0.92) 43.0 43.7 (⨉1.01) +0 crates
cargo 81.4 76.6 (⨉0.94) 129.8 129.7 (⨉1.00) +3 crates
ripgrep 27.2 28.1 (⨉1.03) 58.6 62.3 (⨉1.06) +6 crates
gluon 66.5 61.9 (⨉0.93) 112.7 115.2 (⨉1.02) +35 crates
imag 81.4 100.4 (⨉1.23) 231.5 266.4 (⨉1.15) +79 crates

As you can see, sometimes it is a little faster, but usually it is slower (sometimes much slower).

Here's a few more pieces of data that seemed interesting (of 21098 crates):

Default settings

The current defaults may not be the best. Turning off debug improves speed and reduces disk space, but then you lose good backtraces. It's also questionable if it matters if debug-assertions or overflow-checks are off. Setting opt-level=1 had a noticeable increase in compile time on the few projects I tried, so I left it at 0.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for doing the analysis here!

To make sure I understand this, the PR proposed as-is changes the default build settings for build scripts/procedural macros in both debug/release modes. This means that entire dependency trees rooted in procedural macros and build scripts are now compiled differently, and any previous sharing which happend no longer occurs, accounting for longer build times.

I'm curious if you know if there are some particularly bad "root offenders"? How do crates like crossgen have 161 shared crates (or even imag with 79)? That may be good for evaluating how to move forward on this.

FWIW absolute compile times aren't always the most interesting metric in my opinion. Incremental builds almost always occur because there's previous artifacts and/or build times were already bad enough to motivate tools like sccache which compile all these rlibs super quickly. In that sense I'm personally ok eating a regression here to solve the, what is this point, huge litany of bugs this feature could fix.

debug-assertions = false
codegen-units = 16
panic = 'unwind'
incremental = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be true? (was this copy/pasted from somewhere else that needs an update?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it is false. The default build profile is defined here based on the default here. This is similar to release mode.

I don't have a strong opinion about any of the defaults. I think the theory on this one is that build scripts are rarely modified. But I can see how it would be annoying when you are actively working on it.

Maybe you are thinking of #6564 which hasn't merged, yet? That would change the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nah I just wanted to confirm. I think that we should have incremental turned on by default as the overhead only applies for path dependencies anyway, in this case typically just the build script itself.

I could see this going either way though. Build scripts are typically quite small and fast to compile, which means that incremental isn't a hit for them nor does it really matter too much. I'd personally err on the side of enabling incremental though

debug = false
rpath = false
lto = false
debug-assertions = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little wary about this being false, it seems like this may want to be true by default to help weed out mistakes more quickly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I wouldn't mind making it true. Maybe the same for overflow-checks? I don't have a sense of how much slower that typically makes things, but I suspect it would not be perceivable by most scripts/macros. I think debug is the bigger question of how it should be defaulted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this and overflow-checks should probably default to on, build scripts are typically rarely a bottleneck and if they both of these options can be disabled pretty easily (both from crates.io via changing apis or locally by changing profiles).

For debug I wonder if we could perhaps try setting a default of 1? That means we only generate line tables for backtraces, but no local variable info as no one's really using gdb on these

@ehuss
Copy link
Contributor Author

ehuss commented Jan 22, 2019

To make sure I understand this

It sounds like you understand it well. I'll look into offenders soon.

@ehuss
Copy link
Contributor Author

ehuss commented Jan 25, 2019

Here is some analysis of root offenders: https://gist.github.com/ehuss/0c9fb074d4b8720316b8ede243006f78. I tried to weight them by how often they are used and how many shared dependencies they tend to have. Maybe not the best weighting strategy.

The top offender is phf_codegen which is used often, and has dependencies on 16 crates that are often used as normal dependencies. In the gist you can see things like rocket_codegen and bindgen pull in a ton of generic dependencies that have a high chance of being used as a normal dependency.

imag (Cargo.toml) is weird because it has some large dependencies in both normal and build-dependencies (clap, the entire imag runtime, etc.). I don't know why it is structured that way.

crossgen (Cargo.toml) also has a few very heavy deps in both build-dep/normal-dep. That is, github_auth by itself has 120 shared deps.

Here is a detailed look at cargo-crev: https://gist.github.com/ehuss/a15704fc8c9d9a345a0d71739e3db32e. It's interesting because there isn't one bad offender, but a bunch of them (bindgen, clear_on_drop, failure, phf_codegen, cc, rand).

@alexcrichton
Copy link
Member

Ok thanks for that analysis! I agree it's pretty hard to draw a trend from that. My main conclusion is largely just that the ecosystem of build dependencies is basically the same as normal dependencies, they themselves are built on a number of crates in the ecosystem and there's some big ones and some small ones.

When thinking about the build as a whole, as mentioned before this change is basically irrelevant for incremental builds. It's also largely irrelevant for builds using caching solutions like sccache, so we're only really worried here about completely cold from-scratch builds.

One aspect of those builds I've often noticed is that for larger projects all hardware parallelism is eaten up during the first half-or-so of the build, but the second half is often more serial as dependencies become chained and all the quick crates are out of the way. The relatively small percentage increase in build times you measured above I think may be explainable that the "time to a serial build" is moving back and we're making use of the unused parallelism at that portion of the build to finish up build dependencies. Now of course those same dependencies can also push back the build because the serial chain of crates could depend on everything being finished.

Overall I still personally feel pretty good about this change. Local projects can always reconfigure back to today's configuration if cold builds matter a lot, and otherwise this should provide a general improvement for working with build scripts and procedural macros.

@ehuss
Copy link
Contributor Author

ehuss commented Jan 31, 2019

Spot on. Do you have any thoughts about how to organize the artifact directory? To address something like #1774 it would need to change so that dev/release will share the same build artifacts.

My preference would be to remove the debug/release directory separation. I suspect there might be opposition to that, though it could maybe be done in a backwards compatible fashion with links. From a functional standpoint of using the cargo CLI, the directories aren't relevant (the metadata hash keeps them separate). I'm having a hard time imagining a use case where you need both debug/release artifacts "live" (hard-linked in the root) at the same time.

If that is untenable, a dedicated build directory would make it difficult to share artifacts, but would allow to switch dev/release without rebuilds.

Or it could just stay as-is, which allows for sharing, but causes rebuilds when switching dev/release.

Or maybe some other option, like build artifacts are always in the debug directory even during release, which would allow sharing in dev mode.

@alexcrichton
Copy link
Member

I definitely think we should solve the rebuiding problem, but I think we could either do that by placing output in a new directory or by hashing more into the filename. I'm actually somewhat surprised that their filenames are conflicting today, do you know what's not being hashed to cause the filenames to be different and avoid colliding into the same filename?

We definitely can't easily remove debug/release folders as they're so widely ingrained today. What I think we could do, however, is move towards a world where those folders only contain final output artifacts rather than intermediate ones. Sort of like how we have target/debug/incremental we could just have target/cache which is a dumping ground of all crates from all over the place. No one really has to poke around there unless they're poking at internals in theory!

@ehuss
Copy link
Contributor Author

ehuss commented Jan 31, 2019

their filenames are conflicting today

I'm a little confused. I was saying that they don't conflict, so there should be no reason they need to be in separate directories.

where those folders only contain final output artifacts

Yea, that's what I meant by "backwards compatible fashion with links" — it would keep the debug/release directories and just link final artifacts there for any tools that expect them.

I'll take a look soon at implementing that soon and see if there are any major drawbacks. I expect there to be a lot of little changes throughout the code, but overall to be straightforward. I'd like to do that in a separate PR if that's OK?

@alexcrichton
Copy link
Member

Oh sorry I was misunderstanding the rebuild point. It's not that we're thrashing a cache but the same artifacts are cached in two locations. That doesn't happen today as the settings are basically always different, but after this change the build profile for dev/release is the same so the artifacts are actually the same.

In the long term I think we're going to move to a global build cache for Cargo, so I think it's fine to go ahead and experiment with it ahead of time. I'm thinking something along the lines of "everything stays exactly the same as it is today", but all files are just hard links to a build cache elsewhere. The build cache is just a dump of everything Cargo ever does, compeltely unorganized.

@ehuss
Copy link
Contributor Author

ehuss commented Feb 3, 2019

I implemented a unified deps directory, but ran into some problems dealing with backwards compatibility. I've been trying a few different approaches, but they all have drawbacks.

  • No backwards compatibility. This breaks a few tools and tests. I tried with rustbuild, and it required a few, small changes. How to find binary from integration test? #5758 will need to be fixed first, as there would be no way for integration tests to find executables. (I counted about 60 crates on crates.io that use current_exe from tests to find exes.)
  • A symlink from target/debug/deps to ../deps. However, I don't see a way to create directory links on older versions of Windows.
  • Hard link everything in target/deps/ to target/debug/deps/. However, I believe some scenarios don't support hard links, so this would end up copying everything.
  • Some hybrid approach where target/deps contains everything, target/debug contains links to final artifacts, and target/debug/deps contains links to tests. This will still break some tools like rustbuild, but might minimize the fallout of 5758.

Any ideas?

@alexcrichton
Copy link
Member

If we break very old Windows I think that's fine, I thought that symlink_dir in the standard library would be good enough for Windows versions that we support? (you already can't do things like run cargo on XP).

I don't actually know any systems that don't support hard links on the same filesystem, but have we hit some in the wild we wanted to handle?

I think breaking rustbuild is fine (especially if we see the breakage coming!).

Overall I think we definitely need to preserve backcompat to ensure that the current patterns for finding a test binary works somewhat (although we have broken this before...). Otherwise it should be fine to ignore older Windows and I think it's fine to assume hard links for perf (although I may be forgetting something critical there).

If we only hard link/copy the final binaries that could mitigate the impact of systems without hard links perhaps and overall reduce the amount of traffic on the filesystem?

@ehuss
Copy link
Contributor Author

ehuss commented Feb 7, 2019

break very old Windows

It is fairly recent. Creating symlinks historically required admin permissions until Windows 10 Creators Update (released mid 2017). The reason you can run on older systems is because

A. I don't think we every try to link directories on Windows. I can only think of macos with dSYM.
B. Even if it tries to create a link, it will fall back to fs::copy when it fails.

don't support hard links

I believe some network filesystems do not support it.

Sometime soonish, unless you have any other feedback, I'll try out the hybrid approach and see how it goes.

@alexcrichton
Copy link
Member

Oh sorry right yeah symlinks won't work but I think that directory junctions are supported much further back on Windows, right? (I forget if that's what symlink_dir does or if hard_link does this in libstd...

Hm network filesystems is a bummer... I think the hybrid approach would be best there though long-term!

@ehuss ehuss mentioned this pull request Feb 14, 2019
@bors
Copy link
Contributor

bors commented Feb 20, 2019

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

@ehuss ehuss added the S-blocked label Mar 7, 2019
bors added a commit that referenced this pull request Apr 2, 2019
Include proc-macros in `build-override`.

This adds proc-macros (and their dependencies) to the `build-override` profile setting.  The motivation is that these are all "build time" dependencies, and as such should probably behave the same.  See the discussion on the [tracking issue](rust-lang/rust#48683 (comment)).  My intent is that this paves the way for stabilizing without necessarily waiting for #6577.

The only change here is the line in `with_for_host`, the rest is just renaming for clarity.

This also includes some of the testsuite changes from #6577 to make it easier to check for compiler flags.
@bors
Copy link
Contributor

bors commented Apr 2, 2019

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

bors added a commit that referenced this pull request Dec 2, 2019
Stabilize profile-overrides.

This stabilizes the profile-overrides feature. This was proposed in [RFC 2282](rust-lang/rfcs#2282) and implemented in #5384. Tracking issue is rust-lang/rust#48683.

This is intended to land in 1.41 which will reach the stable channel on Jan 30th.

This includes a new documentation chapter on profiles. See the ["Overrides" section](https://github.com/rust-lang/cargo/blob/9c993a92ce33f219aaaed963bef51fc0f6a7677a/src/doc/src/reference/profiles.md#overrides) in `profiles.md` file for details on what is being stabilized.

Note: The `config-profile` and `named-profiles` features are still unstable.

Closes #6214

**Concerns**
- There is some risk that `build-override` may be confusing with the [proposed future dedicated `build` profile](#6577). There is some more discussion about the differences at rust-lang/rust#48683 (comment). I don't expect it to be a significant drawback. If we proceed later with a dedicated `build` profile, I think we can handle explaining the differences in the documentation. (The linked PR is designed to work with profile-overrides.)
- I don't anticipate any unexpected interactions with `config-profiles` or `named-profiles`.
- Some of the syntax like `"*"` or `build-override` may not be immediately obvious what it means without reading the documentation. Nobody suggested any alternatives, though.
- Configuring overrides for multiple packages is currently a pain, as you have to repeat the settings separately for each package. I propose that we can extend the syntax in the future to allow a comma-separated list of package names to alleviate this concern if it is deemed worthwhile.
- The user may not know which packages to override, particularly for some dependencies that are split across multiple crates. I think, since this is an advanced feature, the user will likely be comfortable with using things like `cargo tree` to understand what needs to be overridden. There is [some discussion](rust-lang/rust#48683 (comment)) in the tracking issue about automatically including a package's dependencies, but this is somewhat complex.
- There is some possibly confusing interaction with the test/bench profile. Because dependencies are always built with the dev/release profiles, overridding test/bench usually does not have an effect (unless specifying a workspace member that is being tested/benched). Overriding test/bench was previously prohibited, but was relaxed when named profiles were added.
- We may want to allow overriding the `panic`, `lto`, and `rpath` settings in the future. I can imagine a case where someone has a large workspace, and wants to override those settings for just one package in the workspace. They are currently not allowed because it doesn't make sense to change those settings for rlibs, and `panic` usually needs to be in sync for the whole profile.
- There are some strange interactions with `dylib` crates detailed in rust-lang/rust#64319. A fix was attempted, but later reverted. Since `dylib` crates are rare (this mostly applied to `libstd`), and a workaround was implemented for `build-std` (it no longer builds a dylib), I'm not too worried about this.
- The interaction with `share-generics` can be quite confusing (see rust-lang/rust#63484). I added a section in the docs that tries to address this concern. It's also possible future versions of `rustc` may handle this better.
- The new documentation duplicates some of the information in the rustc book.  I think it's fine, as there are subtle differences, and it avoids needing to flip back and forth between the two books to learn what the settings do.
@ehuss
Copy link
Contributor Author

ehuss commented Jan 12, 2021

I'm going to close this PR for now. I'm still interested in pursuing this, but it will take more work than I have time for now.

@ehuss ehuss closed this Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants