-
Notifications
You must be signed in to change notification settings - Fork 638
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
Use custom profiles to set LTO and such #6226
Comments
cc @jakmeier for estimator changes. |
Making ‘-D warnings’ the default may be a minor annoyence to developers who now will need to fix warnings even while they work on the feature (as opposed to only having to do that once they are ready to send the code for review) but it does simplify the code a small amount. Issue: near#6226
Turns out this isn’t as trivial as I thought at first. Issue is that with I wonder that maybe it would be better to enable LTO in release profile and introduce dev-release profile equivalent to current release? Just thinking out loud. |
Good point. I'd probably still lean towards for introduction of Ideally, |
I think that release profile should be what we are actually releasing. Anything less is not a release, it's "debug with optimizations" or something like that. |
The issue is that people expect
The issue with that is that what we’re releasing takes ages and heaps of memory to build. This is why we disabled LTO in the first place. And yes, I am aware that I’m the person who removed LTO from release in the first place and now seemingly suggest putting it back in. It’s more than I’m wondering what’s the best option now with custom profiles stabilised. |
Ahh, so our |
For me, it takes less than 10 minutes to link neard on a GCP instance with 16 GiB RAM when using |
My machine used to OOM when trying to `cargo build --release’. The measurement I’ve made back then show LTO build 70% slower with full build and 1400% slower with incremental build:
Things seems to have improved since my machine no longer OOMs (though it does stagger a little when doing ‘make release’) but the issue is real. 10 minutes is a long time especially if you’d like to |
So what’s the consensus here? The three things we can choose from seem to be:
I’m leaning towards the first option. I don’t really see that much benefit of having the custom profile as at the end of the day it doesn’t clean things that much and I also don’t think that differences in LTO vs. non-LTO builds are so big that it would be significant during development. |
OK, I’m gonna close this with no action taken. If anyone has arguments for doing 2 or 3 from my previous comment feel free to reopen. |
This solves two problems: * Makes it easy to produce a fully-optimized binary when doing lto-sensitive benchmarking * Makes it simpler for the estimator to build the binary with the right optimizations, by avoiding hard-coding config into the estimator. Ideally, we'd change the Makefile as well, but we'd rather not break existing `./target/release` layout. In other words, before we had two sources of truth: Makefile and estimator. Now they are Makefile and Cargo.toml, and we also gained a nice cli flag for building fully optimized version: $ cargo b --profile prod -p neard --bin neard cc near#6226
As of this commit: * `--release` is a production profile with full-lto * `--quick-release` is what the old `--release` was: all optimiations except for lto. cc near#6226
As of this commit: * `--release` is a production profile with full-lto * `--quick-release` is what the old `--release` was: all optimiations except for lto. cc near#6226
As of this commit: * `--release` is a production profile with full-lto * `--quick-release` is what the old `--release` was: all optimiations except for lto. cc near#6226
This solves two problems: * Makes it easy to produce a fully-optimized binary when doing lto-sensitive benchmarking * Makes it simpler for the estimator to build the binary with the right optimizations, by avoiding hard-coding config into the estimator. Ideally, we'd change the Makefile as well, but we'd rather not break existing `./target/release` layout. In other words, before we had two sources of truth: Makefile and estimator. Now they are Makefile and Cargo.toml, and we also gained a nice cli flag for building fully optimized version: $ cargo b --profile prod -p neard --bin neard cc #6226 This PR is prompted by the recent confusion about lto: https://near.zulipchat.com/#narrow/stream/345766-pagoda.2Fstorage.2Fflat-storage/topic/Migration.20plan/near/305801512 I don't think this solves the problem, but hopefully existence of `--profile prod` would make it more obvious? Not super sure though, this is more like an RFC than "yes, I think we should have this"
This solves two problems: * Makes it easy to produce a fully-optimized binary when doing lto-sensitive benchmarking * Makes it simpler for the estimator to build the binary with the right optimizations, by avoiding hard-coding config into the estimator. Ideally, we'd change the Makefile as well, but we'd rather not break existing `./target/release` layout. In other words, before we had two sources of truth: Makefile and estimator. Now they are Makefile and Cargo.toml, and we also gained a nice cli flag for building fully optimized version: $ cargo b --profile prod -p neard --bin neard cc #6226 This PR is prompted by the recent confusion about lto: https://near.zulipchat.com/#narrow/stream/345766-pagoda.2Fstorage.2Fflat-storage/topic/Migration.20plan/near/305801512 I don't think this solves the problem, but hopefully existence of `--profile prod` would make it more obvious? Not super sure though, this is more like an RFC than "yes, I think we should have this"
This solves two problems: * Makes it easy to produce a fully-optimized binary when doing lto-sensitive benchmarking * Makes it simpler for the estimator to build the binary with the right optimizations, by avoiding hard-coding config into the estimator. Ideally, we'd change the Makefile as well, but we'd rather not break existing `./target/release` layout. In other words, before we had two sources of truth: Makefile and estimator. Now they are Makefile and Cargo.toml, and we also gained a nice cli flag for building fully optimized version: $ cargo b --profile prod -p neard --bin neard cc #6226 This PR is prompted by the recent confusion about lto: https://near.zulipchat.com/#narrow/stream/345766-pagoda.2Fstorage.2Fflat-storage/topic/Migration.20plan/near/305801512 I don't think this solves the problem, but hopefully existence of `--profile prod` would make it more obvious? Not super sure though, this is more like an RFC than "yes, I think we should have this"
Rust 1.57 stabilized custom profiles feature of cargo:
rust-lang/cargo#9943
That means that we can switch from using env variables for LTO
nearcore/Makefile
Lines 1 to 2 in d1d4559
to using a proper
--profile production
Steps here:
--profile production
instead.The text was updated successfully, but these errors were encountered: