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

Use custom profiles to set LTO and such #6226

Closed
matklad opened this issue Feb 1, 2022 · 10 comments
Closed

Use custom profiles to set LTO and such #6226

matklad opened this issue Feb 1, 2022 · 10 comments
Assignees
Labels
A-build Area: Anything related to the build and configuration process for nearcore. Node Node team T-node Team: issues relevant to the node experience team

Comments

@matklad
Copy link
Contributor

matklad commented Feb 1, 2022

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

export CARGO_PROFILE_RELEASE_CODEGEN_UNITS = 1
export CARGO_PROFILE_RELEASE_LTO = fat

to using a proper --profile production

Steps here:

  • Add custom profile to root Cargo.toml
  • Search all the places that currently set CARGO_PROFILE_RELEASE_LTO (makefile, estimator, maybe something else?)
  • Update them all to pass --profile production instead.
@matklad matklad added the T-node Team: issues relevant to the node experience team label Feb 1, 2022
@matklad
Copy link
Contributor Author

matklad commented Feb 1, 2022

cc @jakmeier for estimator changes.

@bowenwang1996 bowenwang1996 added the A-build Area: Anything related to the build and configuration process for nearcore. label Feb 3, 2022
mina86 added a commit to mina86/nearcore that referenced this issue Feb 7, 2022
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
@mina86
Copy link
Contributor

mina86 commented Feb 7, 2022

Turns out this isn’t as trivial as I thought at first. Issue is that with --profile production the binary ends up at target/production/neard which means that places which refer to target/release would need to be updated as well.

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.

@matklad
Copy link
Contributor Author

matklad commented Feb 8, 2022

Good point. I'd probably still lean towards for introduction of prod profile: today, to build production artifacts, you are going to use a custom, unusual workflow anyway (make), so adding more stuff there doesn't affect conceptual complexity (it's still a custom command!). In contrast, for workflows like "running a test in release mode to access performance" the standard cargo test --release works, and making that into non-standard cargo test --profile dev-release does add to conceptual complexity.

Ideally, dev-release would be a standard Cargo profile but, alas, it isn't

@abacabadabacaba
Copy link
Collaborator

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.

@mina86
Copy link
Contributor

mina86 commented Feb 8, 2022

I'd probably still lean towards for introduction of prod profile: today, to build production artifacts, you are going to use a custom, unusual workflow anyway (make), so adding more stuff there doesn't affect conceptual complexity (it's still a custom command!).

The issue is that people expect make release to put all the files in target/release which will no longer be true with prod profile. I’d rather avoid a situation where someone was testing some code with cargo build --release and later made a release by running make release and copying files from target/release. We can copy the files over in Makefile but then we just made the build commands more complicated for no discernable reason. As you know, there is --out-dir but it’s unstable so we probably don’t want to use it.

I think that release profile should be what we are actually releasing.

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.

@matklad
Copy link
Contributor Author

matklad commented Feb 8, 2022

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.

Ahh, so our Makefile doesn't do "link files from target/release to output directory". Or, rather, we only do this for sandbox in a rather unprincipled way. Yeah, then it's probably better to not change that just because. I am now 0.7 convinced that it's better to introduce dev-release and to mention it in the contributing.md.

@abacabadabacaba
Copy link
Collaborator

The issue with that is that what we’re releasing takes ages and heaps of memory to build.

For me, it takes less than 10 minutes to link neard on a GCP instance with 16 GiB RAM when using make release. So I think that this claim is kinda exaggerated.

@mina86
Copy link
Contributor

mina86 commented Feb 8, 2022

For me, it takes less than 10 minutes to link neard on a GCP instance with 16 GiB RAM when using make release. So I think that this claim is kinda exaggerated.

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:

  • Elapsed time of full builds:

    |          |   -j6 |  -j8 | -j12 | -j24 |
    | lto=off  |  5:16 |      | 3:22 | 2:58 |
    | lto=thin | 11:07 |      | 6:24 | 5:21 |
    | lto=fat  |  8:57 | 7:18 | OOM  | OOM  |
    
  • Elapsed time of incremental builds:

    |          |  -j6 | -j12 | -j24 |
    | lto=off  | 0:10 | 0:10 | 0:10 |
    | lto=thin | 2:32 | 1:22 | 1:09 |
    | lto=fat  | 2:43 | 2:41 | OOM  |
    

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 git bisect. So the question is more whether people do those kinds of things.

@mina86
Copy link
Contributor

mina86 commented Feb 16, 2022

So what’s the consensus here? The three things we can choose from seem to be:

  1. Leave things as they are now.

  2. Use a custom profile for release build. This will remove a few export lines from Makefile but we will have to deal with executable ending up in a different directory. This perhaps wouldn’t be such a big issue except it seems we’ve got validators used to building things through make release and copying files from target/release. So we probably would need to add copying to Makefile but that sounds much more complicated than having a few export statements.

  3. Enable LTO in release profile. This has the issue of increasing the time needed to cargo build --release which might be annoying during development. While for clean builds the increase in time might not be proportionally that big but for incremental builds it can get from several seconds to couple of minutes. Historically, LTO builds also resulted in OOMs and failed builds.

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.

@mina86
Copy link
Contributor

mina86 commented Mar 9, 2022

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.

@mina86 mina86 closed this as completed Mar 9, 2022
matklad added a commit to matklad/nearcore that referenced this issue Oct 25, 2022
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
matklad added a commit to matklad/nearcore that referenced this issue Oct 27, 2022
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
matklad added a commit to matklad/nearcore that referenced this issue Oct 27, 2022
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
matklad added a commit to matklad/nearcore that referenced this issue Oct 27, 2022
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
near-bulldozer bot pushed a commit that referenced this issue Oct 27, 2022
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"
nikurt pushed a commit that referenced this issue Nov 7, 2022
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"
nikurt pushed a commit that referenced this issue Nov 9, 2022
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"
@gmilescu gmilescu added the Node Node team label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build Area: Anything related to the build and configuration process for nearcore. Node Node team T-node Team: issues relevant to the node experience team
Projects
None yet
Development

No branches or pull requests

5 participants