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

add ability to optimize dependencies in dev #2380

Closed
wants to merge 3 commits into from

Conversation

matklad
Copy link
Member

@matklad matklad commented Feb 11, 2016

fixes #1359

Ok, I've tried to implement what we've discussed so far (with "non-local", and a package level toml option).

I suspect that it's likely to be a local decision per-profile (especially if we grow custom profiles)

I agree about "local decision", but I'm not sure about "per-profile". As I see it, several profiles may be used for different packages during a single cargo run. But the decision about profile for deps should be unique withing a single cargo run.

I am afraid, we won't be able to find the right solution here until we have custom profiles, improve dev/test thing and resolve #2085. What would be the conservative option here? Overriding profile for dependencies only in dev mode?

@rust-highfive
Copy link

r? @wycats

(rust_highfive has picked a reviewer for you, use r? to override)

@matklad
Copy link
Member Author

matklad commented Feb 11, 2016

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned wycats Feb 11, 2016
return &self.profiles.dev
}

if let Some(ref id) = self.build_config.deps_profile {
Copy link
Member

Choose a reason for hiding this comment

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

I think this check should probably come first before the check for --release as the release profile may want to switch profiles as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this makes sense. My motivation was that --release should always mean release and compile everything with maximum optimizations. But there should be no harm in allowing more flexibility here.

@alexcrichton
Copy link
Member

Thanks @matklad! Some thoughts of mine:

  • As mentioned on the issue, I'm not sure whether we want this on the [package] section or in the profile section itself. I personally would prefer the profile sections because that's how it's already stored in the backend anyway.
  • Can you write some more tests for negative (e.g. invalid) cases as well?
  • Can you write tests for compiling in release mode but switching back to dev for deps?

This may also be a bit premature for shining the spotlight on profiles, they're still pretty half baked unfortunately.

@matklad
Copy link
Member Author

matklad commented Feb 23, 2016

I personally would prefer the profile sections because that's how it's already stored in the backend anyway.

I've implemented this, but I'm still not sure it is the best option :)

  • profiles now mix some "per compilation unit" and some "global per compilation" info,
  • profiles used to be a flat bunch of attributes, but now they are a graph,
  • the implementation is a bit more complex.

This may also be a bit premature for shining the spotlight on profiles, they're still pretty half baked unfortunately.

Yeah, I see there is a tension between BuildConfig, CompileMode and profiles. Are there any specific issues that can be fixed?

@alexcrichton
Copy link
Member

Thanks! The implementation here looks pretty good to me. I'm not too worried about kinda the "purity" of a Profile today, it seems fine to evolve over time.

Some issues that I know of around profiles are:

  • They're pretty lacking, you can't really do crazy things like configure all options to the compiler.
  • You can't target a specific profile at a particular dependency.
  • You can't make your own profiles
  • Profiles are transmitted to build scripts, but in a pretty lossy fashion.
  • It's somewhat unclear what the "test" and "bench" profiles are as they're only used for the end-products (the literal test runners themselves). This probably isn't the most useful option.

Right now the only real guarantees you have about profiles are:

  • On cargo build, the dev profile is used to compile everything. Build scripts have something like "dev" as the profile passed to them.
  • On cargo build --release, the release profile is used to compile everything. Build scripts have "release" passed to them.

And... that may be it?

@alexcrichton
Copy link
Member

I got a chance to talk about this with @wycats today and the conclusion we reached was:

  • We probably don't want to push on this just yet as rustc fundamentally can't perform this operation. The problem here is that monomorphized code is compiled with downstream options, not how the crate itself was optimized
  • There are a number of deeper problems here which may want to be fixed first. For example faster edit/compile times in release mode will come from incremental compilation. Faster runtimes would likely be more appropriate with an "O1" workflow as opposed to "set this semi-obscure profile option".
  • It's unclear whether this would be recommended because optimized dependencies lose a lot of fidelity in their debug information.

For these reasons I'm going to close for now, but we can continue discussing on the issue!

@matklad matklad deleted the fast-dependencies branch June 9, 2016 10:56
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.

Deprecate the "bench" and "test" profiles Add an option to optimize just dependencies
4 participants