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

Figure out how to add a second "release" profile for "dev+optimized" builds #4189

Open
brson opened this Issue Feb 10, 2019 · 8 comments

Comments

2 participants
@brson
Copy link
Contributor

brson commented Feb 10, 2019

I believe there are two use cases currently covered by the "release" profile: actual release builds and "formal" benchmarking; local development, testing and benchmarking with optimizations.

For the former it is appropriate to turn on all optimizations (e.g. full LTO, no incremental, 1 codegen unit) and turn off anything that hinders optimization.

For the latter I believe it is acceptable to lose a small amount of performance for the sake of compile times, as long as the performance remains consistent across builds. This profile could e.g. use incremental compilation, multiple codegen units, opt-level 2, no LTO. It would still be fast, just not as fast as an aggressively optimized release build.

The first profile is a "real release" profile, the second is a "dev optimized" profile. For the sake of maintaining existing workflows the "dev optimized" would probably be called "release", so that developers can continue using the "--release" flag. The "real release" profile could be called perhaps "real-release" and it would be acceptable for the release build machines to take extra steps to enable it.

The big problem here is that cargo doesn't support custom profiles. There's an issue open for it rust-lang/cargo#2007 but it's reportedly a long way from being implemented.

Related discussion here rust-lang/rust#47745 (comment) along with some suggestions.

It looks like the smartest way to do this currently is with the nigthly-only cargo config profiles. We could throw a config file into e.g the etc/ directory where cargo won't pick it up by default, and have the release builders invoke cargo with that config overriding the "release" profile.

cc @zhouqiang-cl re the impact on the release builders. Any comments or suggestions welcome.

Edit: next steps outlined in #4189 (comment)

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Feb 10, 2019

Unfortunately I don't see any way to tell cargo to use a specific config file on the command line - it seems to have to find it through its hierarchical search. That means that if we created a "real-release" config file that was stored out of the search path in e.g. etc/, then to use it the release builders would need to copy it into place.

Cargo can also be configured with environment variable, which wouldn't require copying a config file, but would still require scripting.

@zhouqiang-cl do either the CI or release builders use the makefile? In other words, if we do extra configuration for the true release build can we put the scripting for that into the makefile?

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Feb 10, 2019

This is a pretty straightforward issue to implement for a new contributor. We're going to create a new "dev-opt" profile along with a new target in the Makefile. For now we'll make the "dev-opt" profile the special case, needing to copy .config files around, but in the future, once there's enough benefit, we'll switch it so that the default release build is "dev-opt".

  • Create etc/cargo-dev-opt-config and copy the existing [profile.release] section from Cargo.toml
  • Add a "dev-opt" Makefile that is the same as the "release" target
  • Modify the "dev-opt" target to copy that file to .cargo/config before building, adding a check that the file doesn't already exist before doing so
  • Modify the "dev-opt" Makefile rule to delete .cargo/config after the build

To start the two configurations will be the same, but as we complete various issues in the "improve compile time" project they will become different.

I'm not sure the best way to verify that the above changes work, but a brute force way would be to temporarily change one of the profiles to do something that will drastically change the resulting binaries, like adding/removing debuginfo; build before and after and check that target/release/tikv-servers size changes reasonably.

Or just make a change that would modify the rustc command line parameters, and verify them with cargo -Vv.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Feb 10, 2019

Note that using config profiles requires passing a -Z flag to cargo.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Feb 11, 2019

This is a prerequisite to completing many of the other compile time issues so needs to be done ASAP.

@da-x

This comment has been minimized.

Copy link

da-x commented Feb 18, 2019

@brson I am trying to promote an "easy-peasy" custom profile solution in Cargo. rust-lang/cargo#6676 , maybe it can help here.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Feb 19, 2019

Thanks for the heads-up @da-x! I hope it's accepted.

@da-x

This comment has been minimized.

Copy link

da-x commented Mar 1, 2019

brson added a commit to brson/tikv that referenced this issue Mar 7, 2019

makefiles: add targets for experimenting with cargo profiles
As part of tikv#4189, this sets up a way to add a 'third' profile beyond the
cargo-default 'dev' and 'release', as well as experiment generally with cargo
profiles by overlaying cargo config files.

Cargo config files contain many options beyond Cargo.toml that can affect build
time.

This setup is intended to be temporary until a satisfying set of profiles is
found, at which point the config files and makefile rules will be integrated
into Cargo.toml and the existing makefile rules.

brson added a commit to brson/tikv that referenced this issue Mar 7, 2019

makefiles: add targets for experimenting with cargo profiles
As part of tikv#4189, this sets up a way to add a 'third' profile beyond the
cargo-default 'dev' and 'release', as well as experiment generally with cargo
profiles by overlaying cargo config files.

Cargo config files contain many options beyond Cargo.toml that can affect build
time.

This setup is intended to be temporary until a satisfying set of profiles is
found, at which point the config files and makefile rules will be integrated
into Cargo.toml and the existing makefile rules.

brson added a commit to brson/tikv that referenced this issue Mar 7, 2019

makefiles: add targets for experimenting with cargo profiles
As part of tikv#4189, this sets up a way to add a 'third' profile beyond the
cargo-default 'dev' and 'release', as well as experiment generally with cargo
profiles by overlaying cargo config files.

Cargo config files contain many options beyond Cargo.toml that can affect build
time.

This setup is intended to be temporary until a satisfying set of profiles is
found, at which point the config files and makefile rules will be integrated
into Cargo.toml and the existing makefile rules.

Signed-off-by: Brian Anderson <andersrb@gmail.com>
@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented Mar 17, 2019

This PR uses .cargo/config to add more profiles: #4324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.