stdenv: Replace "// optionalAttrs" with nullable attr name #430969
Conversation
nix-owners
bot
requested review from
Artturin,
Ericson2314,
RossComputerGuy,
emilazy,
philiptaron and
reckenrode
nixpkgs-ci
bot
added
10.rebuild-darwin: 0
YorikSar
force-pushed
the
remove-attrs-merges-from-mkderivation
branch
from
1aa44ad to
571ec7c
Compare
infinisil
left a comment
infinisil
left a comment
There was a problem hiding this comment.
Damn, those are some really impressive savings without any loss of functionality! 🚀
nixpkgs-ci
bot
added
the
12.approvals: 1
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. |
nixpkgs-ci
bot
added
12.approvals: 2
infinisil
left a comment
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.
YorikSar
force-pushed
the
remove-attrs-merges-from-mkderivation
branch
from
571ec7c to
1d4489d
Compare
YorikSar
left a comment
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. |
3 participants
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.