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

Using profile-overrides results in both optimized and unoptimized versions of the same crate in linked executable #63484

Closed
aclysma opened this issue Aug 12, 2019 · 7 comments
Labels
C-bug Category: This is a bug. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.

Comments

@aclysma
Copy link

aclysma commented Aug 12, 2019

I'm trying to use the "profile-overrides" feature in cargo. The tracking issue for this feature is here: #48683

The documentation for this is here: https://doc.rust-lang.org/cargo/reference/unstable.html#profile-overrides

I'm trying to build my crate without optimizations, and upstream crates with optimizations. (The application runs too slowly to properly test if the upstream crates are not optimized.)

My .toml looks like this:

[profile.dev]
opt-level = 0

[profile.dev.overrides."*"]
opt-level = 3

I have a minimum reproducible example here: https://github.com/aclysma/mre-optimize-dependencies-only

The "slow" crate is nphysics. The minimum reproducible example contains a "main" root crate and a shim crate. Both have nearly the same code, but behave differently:

  • If I call nphysics code from the root crate (that should have optimizations disabled,) it appears in the debugger that the nphysics code is unoptimized.
  • If I call the code from the shim crate (that should have optimizations enabled,) the nphysics code is optimized.

Expected Behavior: the linked binary would have a single implementation for any functions in nphysics, and all call sites would jump to that one address
Observed Behavior: My linked executable appears to have both an unoptimized and optimized version of nphysics, and depending on if the caller is optimized or not, the jump goes to a different address.

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 12, 2019
@matklad
Copy link
Member

matklad commented Aug 15, 2019

I think this is actually expected behavior, at least with the current compilation model. Almost all functions in nphysics are generic, so the compilation to the actual machine code happens not in nphysics, but in the crate that fully specifies generic parameters (instantiates templates, in C++ parlance).

Because both the main crate and the shim crate fully specify nphysics types, you get two copies. Because optimization flags are different, linker doesn't eliminate one.

cc @ehuss for profile overrides. It might be a good idea to mention that profile overrides could inflate the binary size due to un-duplicated monomorphisations. I haven't considered this side-effect of overrides before.

@ehuss
Copy link
Contributor

ehuss commented Aug 15, 2019

Ah, I had been trying to think of how this might happen, that sounds like a plausible explanation! I've been having a similar problem trying to override the profile for std, but I am getting linker errors due to symbol name munging. I wonder if it is the same issue!

@ehuss
Copy link
Contributor

ehuss commented Sep 9, 2019

I just learned about the -Zshare-generics=yes flag (#48779), which will export/link the monomorphization from the optimized crate instead of instantiating it in the local one. It still acts a little wonky (it isn't as fast as being fully optimized), but it is more consistent.

This has the counter-intuitive result that if you change the override from opt-level = 3 to opt-level = 1, it's actually faster, presumably because it enables this mode.

bors added a commit to rust-lang/cargo that referenced this issue Dec 2, 2019
Stabilize profile-overrides.

This stabilizes the profile-overrides feature. This was proposed in [RFC 2282](rust-lang/rfcs#2282) and implemented in #5384. Tracking issue is rust-lang/rust#48683.

This is intended to land in 1.41 which will reach the stable channel on Jan 30th.

This includes a new documentation chapter on profiles. See the ["Overrides" section](https://github.com/rust-lang/cargo/blob/9c993a92ce33f219aaaed963bef51fc0f6a7677a/src/doc/src/reference/profiles.md#overrides) in `profiles.md` file for details on what is being stabilized.

Note: The `config-profile` and `named-profiles` features are still unstable.

Closes #6214

**Concerns**
- There is some risk that `build-override` may be confusing with the [proposed future dedicated `build` profile](#6577). There is some more discussion about the differences at rust-lang/rust#48683 (comment). I don't expect it to be a significant drawback. If we proceed later with a dedicated `build` profile, I think we can handle explaining the differences in the documentation. (The linked PR is designed to work with profile-overrides.)
- I don't anticipate any unexpected interactions with `config-profiles` or `named-profiles`.
- Some of the syntax like `"*"` or `build-override` may not be immediately obvious what it means without reading the documentation. Nobody suggested any alternatives, though.
- Configuring overrides for multiple packages is currently a pain, as you have to repeat the settings separately for each package. I propose that we can extend the syntax in the future to allow a comma-separated list of package names to alleviate this concern if it is deemed worthwhile.
- The user may not know which packages to override, particularly for some dependencies that are split across multiple crates. I think, since this is an advanced feature, the user will likely be comfortable with using things like `cargo tree` to understand what needs to be overridden. There is [some discussion](rust-lang/rust#48683 (comment)) in the tracking issue about automatically including a package's dependencies, but this is somewhat complex.
- There is some possibly confusing interaction with the test/bench profile. Because dependencies are always built with the dev/release profiles, overridding test/bench usually does not have an effect (unless specifying a workspace member that is being tested/benched). Overriding test/bench was previously prohibited, but was relaxed when named profiles were added.
- We may want to allow overriding the `panic`, `lto`, and `rpath` settings in the future. I can imagine a case where someone has a large workspace, and wants to override those settings for just one package in the workspace. They are currently not allowed because it doesn't make sense to change those settings for rlibs, and `panic` usually needs to be in sync for the whole profile.
- There are some strange interactions with `dylib` crates detailed in rust-lang/rust#64319. A fix was attempted, but later reverted. Since `dylib` crates are rare (this mostly applied to `libstd`), and a workaround was implemented for `build-std` (it no longer builds a dylib), I'm not too worried about this.
- The interaction with `share-generics` can be quite confusing (see rust-lang/rust#63484). I added a section in the docs that tries to address this concern. It's also possible future versions of `rustc` may handle this better.
- The new documentation duplicates some of the information in the rustc book.  I think it's fine, as there are subtle differences, and it avoids needing to flip back and forth between the two books to learn what the settings do.
@Enselic Enselic added T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 9, 2023
@Enselic
Copy link
Member

Enselic commented Dec 9, 2023

Triage: Can someone help me move this issue to cargo please? I don't think there is anything actionable by T-compiler here? Bugs with -Zshare-generics=yes would have to be reported separately, I think.

@ehuss
Copy link
Contributor

ehuss commented Dec 9, 2023

@Enselic, I don't think there is anything to do here, due to the way generics are instantiated. Cargo certainly can't do anything about it. This is documented at https://doc.rust-lang.org/cargo/reference/profiles.html#overrides-and-generics, so I'm going to close since I don't think there is anything else to do here.

@ehuss ehuss closed this as not planned Won't fix, can't repro, duplicate, stale Dec 9, 2023
@aclysma
Copy link
Author

aclysma commented Dec 9, 2023

I opened this issue because I'm concerned there could be unsoundness if a function is generated differently and each generated impl references a structure where members may have been compiled out for one of them (say a string member is stripped in release only.) Maybe this is a non-issue because the structs would be seen as different types? But it would be quite subtle if you were serializing that struct with bincode or something like that.

@aclysma
Copy link
Author

aclysma commented Dec 9, 2023

(Oh I guess it was originally the surprise in performance differences, this was 4 years ago. However I do think whether this can lead to unsound behavior should be carefully considered.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants