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

chore: introduce --profile prod #7923

Merged
merged 2 commits into from
Oct 27, 2022
Merged

chore: introduce --profile prod #7923

merged 2 commits into from
Oct 27, 2022

Conversation

matklad
Copy link
Contributor

@matklad matklad commented 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 #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"

@matklad matklad requested review from mina86 and jakmeier October 25, 2022 13:25
@matklad matklad requested a review from a team as a code owner October 25, 2022 13:25
Makefile Outdated
Comment on lines 3 to 4
# This is equivalent to `--profile prod`, but keeps the resulting binary in
# `./target/release/neard`, to not break historical infra relying on that.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like we can do --prefile prod --target-dir target? I remember I was looking into creating a prod profile a while back; don’t recall why we didn’t go with it then. Another option would be to build in target/prod directory and then symlink executables to target/release

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, no, that was you: #6226

Copy link
Contributor

Choose a reason for hiding this comment

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

And again, no, --target-dir won’t do. It’s again issue with --out-dir being unstable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also think there's sadly no great solution here. The way I think about it, --profile prod is the way to build the right binary, and for Makefile we need to use a work-around. Two work-arounds which would work:

  • export and use -r
  • --profile prod and linking afterwards

I would say first-best solution is to put stuff in ./target/neard, so that we are not dependent on Cargo's layout (eg, if we decide to cross-compile stuff today, we'd also observe different target layout).

But this ideal world obviously won't fly as everyone is already depending on stuff being in ./target/release.

Copy link
Contributor

@mina86 mina86 left a comment

Choose a reason for hiding this comment

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

I’m +0 on this one.

Comment on lines 26 to 30
To produce a fully optimize, production binary, run

```console
$ cargo build --profile prod -p neard --bin neard
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth to mention that the binary will end up in target/prod

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Hm, for me personally this is a useful change because I often set these env variables manually when benchmarking.

But if we add this profile and keep everything as it is, it seems we make the overall infra more complicated with duplication of optimization flags, for a small convenience in return. And one still has to remember that --release is not fully optimized. I am not convinced it is worth it.

I wonder, is there a way we can introduce this new profile and make it the new recommended way to build the production binary? The Makefile target could still produce a binary at target/release but maybe we could deprecate it, so that new tools at least will uses the prod profile?

@mina86
Copy link
Contributor

mina86 commented Oct 26, 2022

I wonder, is there a way we can introduce this new profile and make it the new recommended way to build the production binary? The Makefile target could still produce a binary at target/release but maybe we could deprecate it, so that new tools at least will uses the prod profile?

There are three ways. One is that we could use --out-dir parameter for cargo. Unfortunately, that flag is unstable so for the time being it’s arguably not a viable solution. The other way is that we add linking or copying step to Makefile which copies executables from target/prod to target/release. This removes duplication at the cost of complicating Makefile a little. The last option is to just enable full optimisation for --release build and make it so that disabling optimisation requires setting the flags. Here the question is how many people use --release wanting a not fully optimised binary.

@jakmeier
Copy link
Contributor

Enabling full optimization for a release build seems the cleanest to me. We can also add --profile fast (name up for debate) that has the same semantics as the current release build. (no LTO = fast compilation and fast-but-not-production-ready binary)

Here the question is how many people use --release wanting a not fully optimised binary.

Hopefully the number is small enough that we can convince them all to either wait a bit longer or switch to the new profile.

@matklad
Copy link
Contributor Author

matklad commented Oct 27, 2022

Makes sense, flipped the approach to introduce a --quick-release profile.

@jakmeier could you make a judgement call here? I feel your expertise is the best to own our optimization settings

[profile.quick-release]
inherits = "release"
lto = false
codegen-units = 16
Copy link
Contributor Author

Choose a reason for hiding this comment

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

16 is the default Cargo's setting for -r

@mina86
Copy link
Contributor

mina86 commented Oct 27, 2022

@matklad
Copy link
Contributor Author

matklad commented Oct 27, 2022

might also need some updating (though I cannot find where the second document now lives).

The second document is now the fast_builds.md, updated in this PR

@matklad
Copy link
Contributor Author

matklad commented Oct 27, 2022

near/node-docs#57

@matklad matklad requested a review from jakmeier October 27, 2022 17:07
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Yes I think we should do it. Then --release actually produces a release build.
But we should probably announce it in a Zulip channel so people can update their workflow instead of being confused about super slow compilation time on -r. (can be done after merging IMO)

Otherwise, worst case that can happen, I think, is some automated setup starts timing out. That's a risk we can take.


In this PR or a follow-up: Remove this now obsolete code from the params-estimator main.rs

if full {
    cmd.args(&["--env", "CARGO_PROFILE_RELEASE_LTO=fat"])
        .args(&["--env", "CARGO_PROFILE_RELEASE_CODEGEN_UNITS=1"]);
}

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
Copy link
Contributor Author

matklad commented Oct 27, 2022

@near-bulldozer near-bulldozer bot merged commit 7477756 into near:master Oct 27, 2022
nikurt pushed a commit that referenced this pull request 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 pull request 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"
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