Skip to content

buildRustCrate: do not forcibly disable rustc parallelism#274135

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

buildRustCrate: do not forcibly disable rustc parallelism#274135
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@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!
@ghost ghost requested review from figsoda, winterqt and zowoq as code owners December 14, 2023 03:39
@github-actions github-actions bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Dec 14, 2023
@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 person. label Dec 14, 2023
Copy link
Copy Markdown
Member

@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
Copy Markdown
Contributor

lblasc commented Jan 23, 2024

What is the reason for closing this PR ?

@katexochen
Copy link
Copy Markdown
Contributor

katexochen commented Jan 23, 2024

I think this PR was closed in in the light of NixOS/nixpkgs-committers#60 (comment).

@nixos-discourse
Copy link
Copy Markdown

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
Copy Markdown

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 General-purpose programming language emphasizing performance, type safety, and concurrency. 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 person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants