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

buildRustCrate: do not forcibly disable rustc parallelism #274135

Closed
wants to merge 1 commit into from
Closed

buildRustCrate: do not forcibly disable rustc parallelism #274135

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 14, 2023

Our buildRustCrate currently cripples all parallelism, unlike our buildRustPackage. We should not do this.

Our buildRustCrate for sets codegen-units to 1 if unspecified. The codegen-units option controls the amount of process-level parallelism that LLVM employs during the build.

The default is supposed to be 16, but we should simply leave the option unspecified in order to track any future changes to the default. By forcing this to 1 we are disabling all intra-derivation parallelism. This makes buildRustCrate and crate2nix much slower than cargo, for no good reason.

buildRustCrate currently sets `codegen-units` to `1` if unspecified.
This option controls the amount of process-level parallelism that
LLVM employs during the build.

The [default is supposed to be
16](https://doc.rust-lang.org/rustc/codegen-options/index.html#codegen-units).
By forcing this to `1` we are disabling all intra-derivation
parallelism.  This makes `buildRustCrate` and `crate2nix` much
slower than `cargo`; we shouldn't do that!
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 14, 2023
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 14, 2023
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

I followed git blame a bit - this has been set to a static value due to nondeterminism if using NIX_BUILD_CORES in #170981.

The discussion in that same PR mentions the following too:

the difference between codegen-units=1 and codegen-units=16 was a bit less than a factor of 2 of build time on my 24-core machine

It also links to #130309 (comment), which prefers 1 due to performance of the resulting binary.

Your PR description "[…] cripples all parallelism […]" suggests a much higher compilation performance penalty.

I'm also not sure about the implications on runtime performance in real-world scenarios. Do you have any numbers to back both things up?

@ghost ghost closed this Jan 23, 2024
@ghost ghost deleted the pr/crate2nix/dont-disable-parallelism branch January 23, 2024 06:47
@lblasc
Copy link
Contributor

lblasc commented Jan 23, 2024

What is the reason for closing this PR ?

@katexochen
Copy link
Contributor

katexochen commented Jan 23, 2024

I think this PR was closed in in the light of #50105 (comment).

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/crate2nix-setting-codegenunits-for-all-crates/53014/2

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/crate2nix-setting-codegenunits-for-all-crates/53014/3

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: rust 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants