rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.76k stars 2.42k forks source link

LTO behavior depends on root targets #8337

Closed ehuss closed 4 years ago

ehuss commented 4 years ago

Comparing these two commands building Cargo itself with lto='thin':

  1. cargo build --release
  2. cargo build --release --bin cargo

The first command does not engage the LTO codegen optimization added in #8192. On my system, the total build time goes from 187s to 150s.

It is a bit surprising, since the two commands generally produce the same output, so I would not expect them to use different settings.

Would it make sense to be a little smarter about how the root units are chosen? I think the crux of the issue is this line where it starts the lib at Lto::None in the first case. In the second case, it is computed as a dependency with Lto::OnlyBitcode.

alexcrichton commented 4 years ago

I agree that we should probably fix this since it's otherwise a bit surprising that your "main" library is so different from other libraries. I also agree that it's due to that root issue you pointed out.

I think it may be a possible solution to specify all roots as OnlyBitcode? That way we'd guarantee that root libraries are at least compiled.

ehuss commented 4 years ago

Hm. Starting with OnlyBitcode doesn't work in many situations (tests, dylib only, etc.). I'll try to whittle it down, but I'm thinking of moving the if block into a separate function and calling that to prime the initial value. It's quite tricky.

Do you have a preference on how check is handled? AFAIK, it ignores any lto-related flags, so I'm not sure it matters too much.

ehuss commented 4 years ago

Double hmm... Building a project with a dylib dependency with LTO is currently broken. It builds the dylib with -C linker-plugin-lto, but that chokes during linking. Should that be EmbedBitcode or None? I'm not sure it matters too much, but I'm guessing EmbedBitcode?

Also, would you mind too much if we rename the Lto variants, I'm constantly having to look up what they mean. Maybe something like:

Lto::Run              // -C lto
Lto::OnlyBitcode      // -C linker-plugin-lto
Lto::ObjectAndBitcode // no flags
Lto::OnlyObject       // -C embed-bitcode=no
ehuss commented 4 years ago

Just an update, I've been fiddling with this for a while, and I think I have something that should work better. I'll try to work on some more tests tomorrow and see if I can post a PR soon.

alexcrichton commented 4 years ago

Hm for dylib dependencies I think it needs to be None. They don't participate in LTO at all, but IIRC you can't Rust-LTO dependency graphs with dylibs in them, the compiler should reject that?

xobs commented 4 years ago

I'm still experiencing this on 1.46. I had a repo that reproduced the issue on 1.45, and I was just about to file the issue when 1.46 was released. #8349 fixed my test case, however I have since noticed that it does not fix my actual project:

[5:57:06 PM] D:/Code/Xous/core> cargo build --release -p shell
   Compiling cfg-if v0.1.10
   Compiling getrandom v0.1.14
   Compiling log v0.4.11
   Compiling rand_core v0.5.1
   Compiling rand_chacha v0.2.2
   Compiling rand_pcg v0.2.1
   Compiling rand v0.7.3
   Compiling xous-macros v0.1.0 (D:\Code\Xous\core\macros)
   Compiling xous v0.1.0 (D:\Code\Xous\core\xous-rs)
   Compiling graphics-server v0.1.0 (D:\Code\Xous\core\examples\graphics-server)
   Compiling shell v0.1.0 (D:\Code\Xous\core\examples\shell)
    Finished release [optimized + debuginfo] target(s) in 25.41s
[5:57:35 PM] D:/Code/Xous/core> cargo build --release -p log-server
   Compiling cfg-if v0.1.10
   Compiling log v0.4.11
   Compiling xous v0.1.0 (D:\Code\Xous\core\xous-rs)
   Compiling log-server v0.1.0 (D:\Code\Xous\core\examples\log-server)
    Finished release [optimized + debuginfo] target(s) in 15.39s
[5:57:52 PM] D:/Code/Xous/core> cargo build --release -p shell
   Compiling cfg-if v0.1.10
   Compiling getrandom v0.1.14
   Compiling log v0.4.11
   Compiling rand_core v0.5.1
   Compiling rand_chacha v0.2.2
   Compiling rand_pcg v0.2.1
   Compiling rand v0.7.3
   Compiling xous-macros v0.1.0 (D:\Code\Xous\core\macros)
   Compiling xous v0.1.0 (D:\Code\Xous\core\xous-rs)
   Compiling graphics-server v0.1.0 (D:\Code\Xous\core\examples\graphics-server)
   Compiling shell v0.1.0 (D:\Code\Xous\core\examples\shell)
    Finished release [optimized + debuginfo] target(s) in 27.15s
[5:58:21 PM] D:/Code/Xous/core>

If I disable lto="thin" in my Cargo.toml it works correctly:

[5:58:21 PM] D:/Code/Xous/core> cargo build --release -p log-server
   Compiling log-server v0.1.0 (D:\Code\Xous\core\examples\log-server)
    Finished release [optimized + debuginfo] target(s) in 4.14s
[5:58:45 PM] D:/Code/Xous/core> cargo build --release -p shell
   Compiling shell v0.1.0 (D:\Code\Xous\core\examples\shell)
    Finished release [optimized + debuginfo] target(s) in 0.77s
[5:58:47 PM] D:/Code/Xous/core> cargo build --release -p log-server
    Finished release [optimized + debuginfo] target(s) in 0.19s
[5:58:49 PM] D:/Code/Xous/core> cargo build --release -p shell
    Finished release [optimized + debuginfo] target(s) in 0.27s
[5:58:56 PM] D:/Code/Xous/core>

The repository is available at https://github.com/betrusted-io/xous-core and the above commands can be used to demonstrate the issue.

One interesting thing to note is that both log-server and shell depend on xous, which has a feature that depends on log. If I remove this feature (by setting default = [] in xous-rs/Cargo.toml), then the rebuild works just fine with lto enabled.

ehuss commented 4 years ago

@xobs, can you file a new issue? It doesn't seem directly related to this.

Also, briefly tried your repro steps, but it didn't cause any rebuilds.

You can run cargo with the CARGO_LOG=cargo::core::compiler::fingerprint=trace environment variable set to have it explain why it thinks it needs to rebuild.