-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Build host dependencies with opt-level 0 by default #8500
Build host dependencies with opt-level 0 by default #8500
Conversation
This commit updates Cargo's build of host dependencies to build them with optimization level 0 by default instead of matching the profile of the final binary. Since Cargo's inception build dependencies have, by default, been built in a profile that largely matches the profile of the final target artifact. Build dependencies, however, rarely actually need to be optimized and are often executing very small tasks, which means that optimizing them often wastes a lot of build time. A great example of this is procedural macros where `syn` and friends are pretty heavyweight to optimize, and the amount of Rust code they're parsing is typically quite small, so the time spent optimizing rarely comes as a benefit. The goal of this PR is to improve build times on average in the community by not spending time optimizing build dependencies (build scripts, procedural macros, and their transitive dependencies). The PR will not be a universal win for everyone, however. There's some situations where your build time may actually increase: * In some cases build scripts and procedural macros can take quite a long time to run! * Cargo may not build dependencies more than once if they're shared with the main build. This only applies to builds without `--target` where the same crate is used in the final binary as in a build script. In these cases, however, the `build-override` profile has existed for some time know and allows giving a knob to tweak this behavior. For example to get back the previous build behavior of Cargo you would specify, in `Cargo.toml`: [profile.release.build-override] opt-level = 3 or you can configure this via the environment: export CARGO_PROFILE_RELEASE_BUILD_OVERRIDE_OPT_LEVEL=3 There are two notable features we would like to add in the future which would make the impact of a change like this smaller, but they're not implemented at this time (nor do we have concrete plans to implement them). First we would like crates to have a way of specifying they should be optimized by default, despite default profile options. Often crates, like lalrpop historically, have abysmal performance in debug mode and almost always (even in debug builds) want to be built in release mode. The second feature is that ideally crate authors would be able to tell Cargo to minimize the number of crates built, unifying profiles where possible to avoid double-compiling crates. At this time though the Cargo team feels that the benefit of changing the defaults is well worth this change. Neither today nor directly after this change will be a perfect world, but it's hoped that this change makes things less bad!
This comment has been minimized.
This comment has been minimized.
I posted #8501 for an option a crate can use to recommend optimization, and #8502 to avoid building shared build-time and runtime dependencies twice. Suggested release-note language: When building build-time dependencies, which will not be invoked at runtime,
Cargo now defaults to opt-level 0, to reduce build time. If you have a
dependency used only at build time, but it still benefits enough from
optimization to outweigh the time taken to optimize it, you can override
this for an individual crate with:
```toml
[profile.release.package.cratename]
opt-level = 3
```
If you have crates that should almost always be optimized, even when used as a
build-time-only dependency, consider contributing to [cargo issue
8501](https://github.com/rust-lang/cargo/issues/8501) to provide a way to mark
those crates. If you have crates that are used at both build-time and runtime,
consider contributing to [cargo issue
8502](https://github.com/rust-lang/cargo/issues/8502) to build such crates just
once, with optimization. The Cargo team would greatly appreciate your
contributions. |
As discussed in the meeting: |
📌 Commit dc4b695 has been approved by |
☀️ Test successful - checks-actions |
drive-by comment, but we might also want to disable debuginfo generation for host deps, as that takes quite a bit of disk space otherwise. |
That was one thing I considered, yeah, but ended up not doing it just yet. I don't think we'd want to disable debuginfo because it's required for line numbers in backtraces, but |
Update cargo 21 commits in 43cf77395cad5b79887b20b7cf19d418bbd703a9..aa6872140ab0fa10f641ab0b981d5330d419e927 2020-07-13 17:35:42 +0000 to 2020-07-23 13:46:27 +0000 - Update features set in CI. (rust-lang/cargo#8530) - Stabilize -Z crate-versions (rust-lang/cargo#8509) - Fix typo in docs (rust-lang/cargo#8529) - Remove unused CompileMode::all_modes (rust-lang/cargo#8526) - Mask out system core.autocrlf settings before resetting git repos (rust-lang/cargo#8523) - Flag git zlib errors as spurious errors (rust-lang/cargo#8520) - Fix the help display for the target-triple option (rust-lang/cargo#8515) - Check workspace member existence as dir. (rust-lang/cargo#8511) - Bump to 0.48.0, update changelog (rust-lang/cargo#8508) - Apply workspace.exclude to workspace.default-members. (rust-lang/cargo#8485) - Fix nightly tests for intra-doc links. (rust-lang/cargo#8528) - doc: Replace "regenerate" with "revoke" for API tokens (rust-lang/cargo#8510) - Add back Manifest::targets_mut (rust-lang/cargo#8494) - Build host dependencies with opt-level 0 by default (rust-lang/cargo#8500) - Fix freshness checks for build scripts on renamed dirs (rust-lang/cargo#8497) - Add a `-Zbuild-std-features` flag (rust-lang/cargo#8490) - clippy cleanups (rust-lang/cargo#8495) - Fix self-publish script. (rust-lang/cargo#8492) - Ensure `unstable.build-std` works like `-Zbuild-std` (rust-lang/cargo#8491) - Make `cargo metadata` output deterministic (rust-lang/cargo#8489) - Switch to github actions (rust-lang/cargo#8467)
Sorry, bit behind on reviews. I finally had a chance to look at this. This doesn't seem to work if the user has not explicitly set a profile, was that intended? FWIW, I ran some benchmarks with a patched cargo on a handful of projects, and it seemed to be uniformly faster. |
Oh dear that is indeed a bug, my mistake! I think I see a fix as well, let me try to get that with a test up tomorrow |
This fixes an issue where rust-lang#8500 didn't quite work as expected, since it only worked if a crate had a `[profile.release]` section.
Fix O0 build scripts by default without `[profile.release]` This fixes an issue where #8500 didn't quite work as expected, since it only worked if a crate had a `[profile.release]` section.
Fix non-determinism with new feature resolver. This fixes a problem where Cargo was getting confused when two units were identical, but linked to different dependencies. Cargo generally assumes `Unit` is unique, but the new feature resolver can introduce a situation where two identical `Unit`s need to link to different dependencies. In particular, when building without the `--target` flag, the difference between a host unit and a target unit is not captured in the `Unit` structure. A dependency shared between normal dependencies and build dependencies can need to link to a second shared dependency whose features may be different. The solution here is to build the unit graph pretending that `--target` was specified. Then, after the graph has been built, a second pass replaces `CompileKind::Target(host)` with `CompileKind::Host`, and adds a hash of the dependencies to the `Unit` to ensure it stays unique when necessary. This is done to ensure that dependencies are shared if possible. I did a little performance testing, and I couldn't measure an appreciable difference. I also ran the tests in a loop for a few hours without problems. An alternate solution here is to assume `--target=host` if `--target` isn't specified, and then have some kind of backwards-compatible linking in the `target` directory to retain the old directory layout. However, this would result in building shared host/normal dependencies twice. For *most* projects, this isn't a problem. This already happens when `--target` is specified, or `--release` is used (due to #8500). I'm just being very cautious because in a few projects this can be a large increase in build times. Maybe some day in the future we can be more bold and force this division, but I'm a little hesitant to make that jump. Fixes #8549
If I set:
would that affect the build dependencies as well? |
Yes, but only if you don't pass |
Pkgsrc changes: * Remove patches now integrated upstream, many related to SunOS / Illumos. * The LLVM fix for powerpc is also now integrated upstream. * Adapt those patches where the source has moved or parts are integrated. * The randomness patches no longer applies, and I could not find where those files went... * Provide a separate bootstrap for NetBSD/powerpc 9.0, since apparently the C++ ABI is different from 8.0. Yes, this appears to be specific to the NetBSD powerpc ports. Upstream changes: Version 1.47.0 (2020-10-08) ========================== Language -------- - [Closures will now warn when not used.][74869] Compiler -------- - [Stabilized the `-C control-flow-guard` codegen option][73893], which enables [Control Flow Guard][1.47.0-cfg] for Windows platforms, and is ignored on other platforms. - [Upgraded to LLVM 11.][73526] - [Added tier 3\* support for the `thumbv4t-none-eabi` target.][74419] - [Upgrade the FreeBSD toolchain to version 11.4][75204] - [`RUST_BACKTRACE`'s output is now more compact.][75048] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [`CStr` now implements `Index<RangeFrom<usize>>`.][74021] - [Traits in `std`/`core` are now implemented for arrays of any length, not just those of length less than 33.][74060] - [`ops::RangeFull` and `ops::Range` now implement Default.][73197] - [`panic::Location` now implements `Copy`, `Clone`, `Eq`, `Hash`, `Ord`, `PartialEq`, and `PartialOrd`.][73583] Stabilized APIs --------------- - [`Ident::new_raw`] - [`Range::is_empty`] - [`RangeInclusive::is_empty`] - [`Result::as_deref`] - [`Result::as_deref_mut`] - [`Vec::leak`] - [`pointer::offset_from`] - [`f32::TAU`] - [`f64::TAU`] The following previously stable APIs have now been made const. - [The `new` method for all `NonZero` integers.][73858] - [The `checked_add`,`checked_sub`,`checked_mul`,`checked_neg`, `checked_shl`, `checked_shr`, `saturating_add`, `saturating_sub`, and `saturating_mul` methods for all integers.][73858] - [The `checked_abs`, `saturating_abs`, `saturating_neg`, and `signum` for all signed integers.][73858] - [The `is_ascii_alphabetic`, `is_ascii_uppercase`, `is_ascii_lowercase`, `is_ascii_alphanumeric`, `is_ascii_digit`, `is_ascii_hexdigit`, `is_ascii_punctuation`, `is_ascii_graphic`, `is_ascii_whitespace`, and `is_ascii_control` methods for `char` and `u8`.][73858] Cargo ----- - [`build-dependencies` are now built with opt-level 0 by default.][cargo/8500] You can override this by setting the following in your `Cargo.toml`. ```toml [profile.release.build-override] opt-level = 3 ``` - [`cargo-help` will now display man pages for commands rather just the `--help` text.][cargo/8456] - [`cargo-metadata` now emits a `test` field indicating if a target has tests enabled.][cargo/8478] - [`workspace.default-members` now respects `workspace.exclude`.][cargo/8485] - [`cargo-publish` will now use an alternative registry by default if it's the only registry specified in `package.publish`.][cargo/8571] Misc ---- - [Added a help button beside Rustdoc's searchbar that explains rustdoc's type based search.][75366] - [Added the Ayu theme to rustdoc.][71237] Compatibility Notes ------------------- - [Bumped the minimum supported Emscripten version to 1.39.20.][75716] - [Fixed a regression parsing `{} && false` in tail expressions.][74650] - [Added changes to how proc-macros are expanded in `macro_rules!` that should help to preserve more span information.][73084] These changes may cause compiliation errors if your macro was unhygenic or didn't correctly handle `Delimiter::None`. - [Moved support for the CloudABI target to tier 3.][75568] - [`linux-gnu` targets now require minimum kernel 2.6.32 and glibc 2.11.][74163] Internal Only -------- - [Improved default settings for bootstrapping in `x.py`.][73964] You can read details about this change in the ["Changes to `x.py` defaults"](https://blog.rust-lang.org/inside-rust/2020/08/30/changes-to-x-py-defaults.html) post on the Inside Rust blog. - [Added the `rustc-docs` component.][75560] This allows you to install and read the documentation for the compiler internal APIs. (Currently only available for `x86_64-unknown-linux-gnu`.) [1.47.0-cfg]: https://docs.microsoft.com/en-us/windows/win32/secbp/control-flow-guard [76980]: rust-lang/rust#76980 [75048]: rust-lang/rust#75048 [74163]: rust-lang/rust#74163 [71237]: rust-lang/rust#71237 [74869]: rust-lang/rust#74869 [73858]: rust-lang/rust#73858 [75716]: rust-lang/rust#75716 [75908]: rust-lang/rust#75908 [75516]: rust-lang/rust#75516 [75560]: rust-lang/rust#75560 [75568]: rust-lang/rust#75568 [75366]: rust-lang/rust#75366 [75204]: rust-lang/rust#75204 [74650]: rust-lang/rust#74650 [74419]: rust-lang/rust#74419 [73964]: rust-lang/rust#73964 [74021]: rust-lang/rust#74021 [74060]: rust-lang/rust#74060 [73893]: rust-lang/rust#73893 [73526]: rust-lang/rust#73526 [73583]: rust-lang/rust#73583 [73084]: rust-lang/rust#73084 [73197]: rust-lang/rust#73197 [72488]: rust-lang/rust#72488 [cargo/8456]: rust-lang/cargo#8456 [cargo/8478]: rust-lang/cargo#8478 [cargo/8485]: rust-lang/cargo#8485 [cargo/8500]: rust-lang/cargo#8500 [cargo/8571]: rust-lang/cargo#8571 [`Ident::new_raw`]: https://doc.rust-lang.org/nightly/proc_macro/struct.Ident.html#method.new_raw [`Range::is_empty`]: https://doc.rust-lang.org/nightly/std/ops/struct.Range.html#method.is_empty [`RangeInclusive::is_empty`]: https://doc.rust-lang.org/nightly/std/ops/struct.RangeInclusive.html#method.is_empty [`Result::as_deref_mut`]: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.as_deref_mut [`Result::as_deref`]: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.as_deref [`TypeId::of`]: https://doc.rust-lang.org/nightly/std/any/struct.TypeId.html#method.of [`Vec::leak`]: https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#method.leak [`f32::TAU`]: https://doc.rust-lang.org/nightly/std/f32/consts/constant.TAU.html [`f64::TAU`]: https://doc.rust-lang.org/nightly/std/f64/consts/constant.TAU.html [`pointer::offset_from`]: https://doc.rust-lang.org/nightly/std/primitive.pointer.html#method.offset_from
- Update dependencies This was done manually, but I used a tool to help me find the outdated versions: 1. `cargo install cargo-outdated`. This is a tool to detect dependencies that are out of date. 2. `cargo outdated --root-deps-only`. This shows the top-level dependencies, without showing transient dependencies (like `serial_test_derive` and `clang-sys`). 3. Fix the breakage from updating. In particular I found that `rt-core` was removed in tokio 1.0: ``` > cargo check Updating crates.io index error: failed to select a version for `tokio`. ... required by package `yottadb v1.1.0 (/home/joshua/src/yottadb/YDBRust)` versions that meet the requirements `^1` are: 1.0.1 the package `yottadb` depends on `tokio`, with features: `rt-core` but `tokio` does not have these features. ``` and that `rand` had a breaking change: ``` error[E0061]: this function takes 1 argument but 2 arguments were supplied --> examples/random_walk.rs:29:19 | 29 | let val = rng.gen_range(0, 2); | ^^^^^^^^^ - - supplied 2 arguments | | | expected 1 argument ``` I fixed those errors and also cleaned up `get_global` a little while I was at it. - This also fixes the `Context` doc-test, which, again, was failing for the wrong reasons. The previous error was ``` error[E0308]: mismatched types --> src/context_api/mod.rs:154:19 | 12 | ctx.tp(|_| Ok(()), "BATCH", &[]) | ^^ expected enum `TransactionStatus`, found `()` ``` which is left over from https://gitlab.com/YottaDB/Lang/YDBRust/-/merge_requests/65 (a full 9 months ago!). I fixed it to now give the correct error: ``` failures: ---- src/context_api/mod.rs - context_api::Context (line 144) stdout ---- error[E0277]: `Rc<RefCell<context_api::ContextInternal>>` cannot be shared between threads safely --> src/context_api/mod.rs:151:1 | 10 | tokio::spawn(async { | ^^^^^^^^^^^^ `Rc<RefCell<context_api::ContextInternal>>` cannot be shared between threads safely | ::: /home/joshua/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.0.1/src/task/spawn.rs:128:21 | 128 | T: Future + Send + 'static, | ---- required by this bound in `tokio::spawn` | = help: within `yottadb::context_api::Context`, the trait `Sync` is not implemented for `Rc<RefCell<context_api::ContextInternal>>` = note: required because it appears within the type `yottadb::context_api::Context` = note: required because of the requirements on the impl of `Send` for `&yottadb::context_api::Context` = note: required because it appears within the type `[static generator@src/context_api/mod.rs:10:20: 13:2 _]` = note: required because it appears within the type `from_generator::GenFuture<[static generator@src/context_api/mod.rs:10:20: 13:2 _]>` = note: required because it appears within the type `impl Future` ``` The issue was that Rust does not have a stable way to test that a test gives a *specific* error instead of failing to compile. On nightly there is a way: https://doc.rust-lang.org/rustdoc/unstable-features.html#error-numbers-for-compile-fail-doctests and I added it, but it will only have an effect on nightly toolchains. Since [we test on nightly](https://gitlab.com/YottaDB/Lang/YDBRust/-/blob/b7980d49be62e890ac3246573bf02e1239086994/.gitlab-ci.yml#L67), the hope is we'll notice relatively quickly if it starts to fail for the wrong reason again; even though `allow_failure` is set, it will still show up in the 'pipeline' view on GitLab I think. Note that you can find these error messages by removing `compile_fail` then running `cargo test --doc context_api::Context` locally, just remember to add back `compile_fail` when you're done. All the other changes to context_api are just cleanups (e.g. changing `#[macro_use]` to a normal path import). - Increase minimum supported version Bindgen 0.56 no longer supports rust 1.34, so the MSRV was also increased to 1.40. Unfortunately, this exposed an issue with cargo: although `profile.release.package."*"` was ignored on 1.34 (since it was unsupported), in 1.40 it gives an error that it requires nightly features. For now, remove the feature. It may be possible to enable it for toolchains that support it by setting `CARGO_PROFILE_RELEASE_BUILD_OVERRIDE_OPT_LEVEL=0` in the environment, but that would require a toolchain of at least version 1.41, which I'm not sure is currently the case for our internal test suite. Note that the feature was enabled by default in Cargo 1.49 (rust-lang/cargo#8500), so there is a very small window of toolchains where the environment variable would have an effect.
2788: build: customize `opt-level` to avoid building some crates twice r=quake,driftluo a=yangby-cryptape ### Purpose Since "Cargo PR-8500" merged, as "Cargo Issue-8502" said: "if a crate will also be built as a runtime dependency, that would result in Cargo building it twice, once with opt-level 0 and once with opt-level 3." So, in this PR I customize the `opt-level` to the default values, to make sure each crates only be compiled once. ### Results When `make prod` for `v0.42.0`: - This PR can reduce the number of compiled dependencies 9.56% (`596 -> 544`). - This PR can reduce the size of target directory 14.2% (`1_813_100 bytes -> 1_587_780 bytes`). - Save total compile time. **Notice: the size of final binary won't be chaneged.** ### References - [Cargo PR-8500: Build host dependencies with opt-level 0 by default](rust-lang/cargo#8500) - [Cargo Issue-8502: Support opt-level ranges to avoid building twice with different opt-level values](rust-lang/cargo#8502) - [Cargo Reference / Default profiles](https://doc.rust-lang.org/cargo/reference/profiles.html#default-profiles) Co-authored-by: Boyu Yang <yangby@cryptape.com>
This commit updates Cargo's build of host dependencies to build them
with optimization level 0 by default instead of matching the profile of
the final binary.
Since Cargo's inception build dependencies have, by default, been built
in a profile that largely matches the profile of the final target
artifact. Build dependencies, however, rarely actually need to be
optimized and are often executing very small tasks, which means that
optimizing them often wastes a lot of build time. A great example of
this is procedural macros where
syn
and friends are pretty heavyweightto optimize, and the amount of Rust code they're parsing is typically
quite small, so the time spent optimizing rarely comes as a benefit.
The goal of this PR is to improve build times on average in the
community by not spending time optimizing build dependencies (build
scripts, procedural macros, and their transitive dependencies). The PR
will not be a universal win for everyone, however. There's some
situations where your build time may actually increase:
long time to run!
the main build. This only applies to builds without
--target
wherethe same crate is used in the final binary as in a build script.
In these cases, however, the
build-override
profile has existed forsome time know and allows giving a knob to tweak this behavior. For
example to get back the previous build behavior of Cargo you would
specify, in
Cargo.toml
:or you can configure this via the environment:
There are two notable features we would like to add in the future which
would make the impact of a change like this smaller, but they're not
implemented at this time (nor do we have concrete plans to implement
them). First we would like crates to have a way of specifying they
should be optimized by default, despite default profile options. Often
crates, like lalrpop historically, have abysmal performance in debug
mode and almost always (even in debug builds) want to be built in
release mode. The second feature is that ideally crate authors would be
able to tell Cargo to minimize the number of crates built, unifying
profiles where possible to avoid double-compiling crates.
At this time though the Cargo team feels that the benefit of changing
the defaults is well worth this change. Neither today nor directly after
this change will be a perfect world, but it's hoped that this change
makes things less bad!