stdenv: Replace "// optionalAttrs" with nullable attr name#430969
stdenv: Replace "// optionalAttrs" with nullable attr name#430969infinisil merged 1 commit intoNixOS:masterfrom
Conversation
1aa44ad to
571ec7c
Compare
infinisil
left a comment
There was a problem hiding this comment.
Damn, those are some really impressive savings without any loss of functionality! 🚀
There was a problem hiding this comment.
Future: This function itself needs to be on the chopping block -- spot its use down below, where it can be inlined into the two uses.
There was a problem hiding this comment.
I don’t think inlining it would yield much benefit - it’s called from three separate places with different arguments and there is no repetition in them. But doesn’t hurt to try :)
There was a problem hiding this comment.
This condition would definitely be more clear if it were extracted to a variable in the let block, ideally near the definition of hardeningDisable', defaultHardeningFlags, etc.
Unrelated to this PR, but hardeningDisable' has a unique which looks performance intensive and would be great to remove if it can be done without incident. I suspect the order of this list doesn't matter.
|
@YorikSar, I'm very OK merging this as-is then doing updates in some future PR. There's a good deal more micro-optimizations to be done, I think. |
infinisil
left a comment
There was a problem hiding this comment.
Moving definitions to a let block is better for a separate PR, so we can independently evaluate the performance difference
As in NixOS#430132, it saves a lot of set allocations and merge operations, but makes code harder to read. I've tried introducing a function like this to make code cleaner, but it increases number of envs and space taken by them significantly: optionalAttr = cond: name: if cond then name else null; I've also tried applying this logic to the section with isDarwin, but it makes things measurably worse for x86_64-linux eval.
571ec7c to
1d4489d
Compare
YorikSar
left a comment
There was a problem hiding this comment.
I've addressed the comment about the contend addressed comment. I want to address the rest of them in a separate PR.
There was a problem hiding this comment.
I’ve opened another PR optimising this part at #431756, please take a look.
@philiptaron I addressed a couple of your comments there, although I hoped to cover more of them.
There was a problem hiding this comment.
I don’t think inlining it would yield much benefit - it’s called from three separate places with different arguments and there is no repetition in them. But doesn’t hurt to try :)
|
Thanks! I'm really excited by your work here. I deeply appreciate it, and its ability to make the experience for literally every single package in Nixpkgs better. |
…tr names Follow-up to NixOS#430969. Replace remaining `// optionalAttrs` patterns with `${if cond then "name" else null} = value;` to avoid per-derivation closure allocations and `//` merge operations. Changes: - make-derivation.nix: inline Darwin, Windows, outputChecks blocks and the `env'`/`__structuredAttrs` patterns in mkDerivationSimple - trivial-builders: inline optionalAttrs in runCommandWith and symlinkJoin
…tr names Follow-up to NixOS#430969. Replace remaining `// optionalAttrs` patterns with `${if cond then "name" else null} = value;` to avoid per-derivation closure allocations and `//` merge operations. Changes: - make-derivation.nix: inline Darwin, Windows, outputChecks blocks and the `env'`/`__structuredAttrs` patterns in mkDerivationSimple - trivial-builders: inline optionalAttrs in runCommandWith and symlinkJoin
As in #430132, it saves a lot of set allocations and merge operations, but makes code harder to read.
I've tried introducing a function like this to make code cleaner, but it increases number of envs and space taken by them significantly:
I've also tried applying this logic to the section with
isDarwin, but it makes things measurably worse forx86_64-linuxeval.Here are significant metrics gathered for
x86_64-linuxeval on my machines:Things done
Add a 👍 reaction to pull requests you find important.