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

Cargo does not install binaries with the profiles as defined in the installed package #11599

Closed
Robbepop opened this issue Jan 19, 2023 · 8 comments
Labels
C-bug Category: bug

Comments

@Robbepop
Copy link

Robbepop commented Jan 19, 2023

Problem

Today I released a CLI tool for my Wasm interpreter: https://crates.io/crates/wasmi_cli

The entire tool depends heavily on the right Cargo profile to be used during compilation, namely:

[profile.release]
lto = "fat"
codegen-units = 1

Otherwise we see performance regressions of 50%-400%.

Cargo docs state that the --release profile is used by default for installs.
However, when installing the wasmi CLI tool via cargo install wasmi_cli and then benchmark it against a local build cargo run --release --package wasmi_cli -- etc.. I see very big differences in performance which leads me to the guess that Cargo does not apply the proper profiles to installed crates but uses the default configurations instead.

This bug is critical to the performance of the wasmi CLI application.

Steps

  1. Install wasmi_cli via cargo install wasmi_cli
  2. Clone the wasmi repository: git clone https://github.com/paritytech/wasmi.git
  3. Set working directory to the wasmi repository: cd wasmi
  4. Benchmark the installed wasmi CLI tool: time wasmi_cli ./crates/wasmi/benches/wat/fibonacci.wat fib_iterative 100000000
  5. Benchmark the CLI from the repo: time cargo run --package wasmi_cli --profile bench ./crates/wasmi/benches/wat/fibonacci.wat fib_iterative 100000000

Observe that step 4. takes way more time than step 5.
Works with other examples as well.
For example using the recursive version of fibonacci: ./crates/wasmi/benches/wat/fibonacci.wat fib_recursive 40

Possible Solution(s)

Cargo should use the profiles as defined in the installed package and not the default settings.

Notes

How did I know that this is probably because Cargo uses the default release settings for installation? I simply compiled the repository wasmi_cli via the default release settings and then both performed equally bad.

Version

cargo 1.66.0 (d65d197ad 2022-11-15)
@Robbepop Robbepop added the C-bug Category: bug label Jan 19, 2023
@weihanglo
Copy link
Member

Just looked into Cargo.toml of wasmi_cli v0.23.0 from https://github.com/paritytech/wasmi/blob/v0.23.0/crates/cli/Cargo.toml, there is no [profile.release] being written. In step 5 you specified profile bench instead of release. It is likely that cargo install wasmi_cli just picks the default release profile, since no one overwrote the default. Maybe I miss any context?

@ehuss
Copy link
Contributor

ehuss commented Jan 19, 2023

Thanks for the report! The issue is that workspace members do not pick up profiles from a workspace when published. This issue is tracked in #8264, so closing as a duplicate of that.

Unfortunately I can't offhand think of a good fix (other than duplicating the profile when publishing). It would definitely be something that would be good to fix.

@ehuss ehuss closed this as not planned Won't fix, can't repro, duplicate, stale Jan 19, 2023
@Robbepop
Copy link
Author

Just looked into Cargo.toml of wasmi_cli v0.23.0 from https://github.com/paritytech/wasmi/blob/v0.23.0/crates/cli/Cargo.toml, there is no [profile.release] being written. In step 5 you specified profile bench instead of release. It is likely that cargo install wasmi_cli just picks the default release profile, since no one overwrote the default. Maybe I miss any context?

Thanks for the reply. Before publishing wasmi_cli I wrote the following into the root:

[profile.release]
lto = "fat"
codegen-units = 1

Also I tried installing wasmi_cli using --profile bench which is specified in the wasmi_cli repo just like the above profile.release but that changed nothing with the performance problem.

@Robbepop
Copy link
Author

Thanks for the report! The issue is that workspace members do not pick up profiles from a workspace when published. This issue is tracked in #8264, so closing as a duplicate of that.

Unfortunately I can't offhand think of a good fix (other than duplicating the profile when publishing). It would definitely be something that would be good to fix.

Thanks a lot for the link to the other issue. I will try out that fix. Could be good enough as a temporary solution.

@Robbepop
Copy link
Author

Just wanted to write here that the suggestion by @ehuss actually worked.
The fix for now is to manually copy over the [profile.release] settings from the workspace Cargo.toml to the CLI Cargo.toml before publishing a new version.
Now the CLI application performance is as expected. :)

@Gor-Madatyan
Copy link

@Robbepop ,

Just wanted to write here that the suggestion by @ehuss actually worked. The fix for now is to manually copy over the [profile.release] settings from the workspace Cargo.toml to the CLI Cargo.toml before publishing a new version. Now the CLI application performance is as expected. :)


I also checked it and tried to do these actions and publish the package, but I got

Warning:

warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   ROOT\crates\__CRATE_NAME__\Cargo.toml
workspace: ROOT\Cargo.toml

at the time of writing this message, simply copying profiles from root Cargo.toml into Cargo.toml of the package is not fixing the problem for me.

could you tell us a little more about how you solved it? The link that gave @ehuss (#8264) just said that in the next edition the behavior of the compiler may change

@Robbepop
Copy link
Author

Robbepop commented Jul 9, 2023

@MAKA-Rustcean I too get that warning when applying this fix and yet it works and properly applies the optimizations. I think the warning is unfortunate given the known bug. However, since this is a bug a warning is fair in order for people to rightfully get confused and eventually landing here and push awareness of this issue. :)

@Gor-Madatyan
Copy link

Gor-Madatyan commented Jul 9, 2023

@MAKA-Rustcean I too get that warning when applying this fix and yet it works and properly applies the optimizations. I think the warning is unfortunate given the known bug. However, since this is a bug a warning is fair in order for people to rightfully get confused and eventually landing here and push awareness of this issue. :)


@Robbepop thank you for the really useful info!!!

gligneul added a commit to OffchainLabs/cargo-stylus that referenced this issue Jan 10, 2025
The replay command wasn't working on MacOS because the HostIO symbols
were striped from cargo-stylus. To fix this issue, we add a profile
release to the workspace and cargo-stylus binary crate.

This fixes introduces the following warning:
warning: profiles for the non root package will be ignored

However, there is workaround to this warning. More information here:
* rust-lang/cargo#8264
* rust-lang/cargo#11599
gligneul added a commit to OffchainLabs/cargo-stylus that referenced this issue Jan 10, 2025
The replay command wasn't working on MacOS because the HostIO symbols
were striped from cargo-stylus. To fix this issue, we have to disable
stripping in the release profile.

This fixes introduces the following warning:

    warning: profiles for the non root package will be ignored

We have to edit the profile in the non-root package because the root
package is ignored during cargo install. Otherwise, the solution won't
work after the package is published to crates.io. More information about
this issue is available here:

* rust-lang/cargo#8264
* rust-lang/cargo#11599
rauljordan pushed a commit to OffchainLabs/cargo-stylus that referenced this issue Jan 14, 2025
* Fix replay command in MacOS

The replay command wasn't working on MacOS because the HostIO symbols
were striped from cargo-stylus. To fix this issue, we have to disable
stripping in the release profile.

This fixes introduces the following warning:

    warning: profiles for the non root package will be ignored

We have to edit the profile in the non-root package because the root
package is ignored during cargo install. Otherwise, the solution won't
work after the package is published to crates.io. More information about
this issue is available here:

* rust-lang/cargo#8264
* rust-lang/cargo#11599

* Fix missing symbols in MacOS

Add #[used] static variables pointing to the exported functions so the
linked doesn't remove them from the final binary.

* Revert "Fix replay command in MacOS"

This reverts commit b489394.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

4 participants