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

Avoid ever building rustc_codegen_spirv more than once in release mode. #546

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Mar 29, 2021

This PR allows these 3 commands to share the same rustc_codegen_spirv build (this also means spirv-tools-sys also only compiles all the C++ code only once, which can itself take a significant amount of time):

# Examples (via build dependency).
cargo build --release -p example-runner-wgpu

# Old tests (via direct dependency).
cargo test --release -p spirv-builder

# New tests (also direct dependency, but also implicitly release mode).
cargo compiletest

Sadly, I'm not aware of relevant automation (though one could already exist), but here's what I found:

  • between regular dependencies and build dependencies, features aren't always unified, and these differed:
    • syn (used by serde_derive -> serde -> rustc_codegen_spirv)
    • libc (used by jobserver -> cc -> spirv-tools-sys -> rustc_codegen_spirv)
    • hashbrown (used by indexmap -> rustc_codegen_spirv)
    • num_traits (used by rspirv -> rustc_codegen_spirv)
  • tests/Cargo.toml (cargo compiletest) didn't have the system of use-{installed,compiled}-tools features that spirv-builder has, resulting in an extra --cfg 'feature="default"' passed when building rustc_codegen_spirv
  • our [profile.release.build-override] contains codegen-units = 16, and that is the default for release mode except the actual default is baked into rustc, so Cargo doesn't actually normally pass it in release mode

I'm not really happy with just fixing it right now, we should try to come up with a test for "did we waste time building everything some things twice" (e.g. checking the target dir for multiple copies of some crates?).

But I don't even think we build anything (other than cargo compiletest) in release mode on CI.
We did try release mode CI in #494, but it took longer and I now think it might be the duplication.
Since we're already running cargo compiletest on CI, I suspect that if we land this PR we should be able to move to release mode, and see a benefit. In fact I might just test that myself once we have data for this PR itself.

@eddyb
Copy link
Contributor Author

eddyb commented Mar 29, 2021

A silly way to enforce this doesn't regress would be to hash librustc_codegen_spirv.so when first built, then require any further commands to preserve the hash. Not sure if it makes sense without --release in CI (a la #547), though.

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