stdenv: Remove extra merge operator in meta #430132
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This operator merges two attrsets without any conditions, and leads to more operations and allocations.
nix-owners
bot
requested review from
Artturin,
Ericson2314,
RossComputerGuy,
emilazy,
philiptaron and
reckenrode
nixpkgs-ci
bot
added
10.rebuild-darwin: 0
Contributor
Author
|
I've decided to remove all merges that don't involve user input, and filter resulting attributes in one |
philiptaron
requested changes
Aug 1, 2025
pkgs/stdenv/generic/check-meta.nix
Outdated
Contributor
There was a problem hiding this comment.
unused; you made use of builtins.removeAttrs below.
Contributor
Author
There was a problem hiding this comment.
Yep, change got stuck in my stage. Fixed now.
Contributor
There was a problem hiding this comment.
Will be good to merge once this is removed again as it is unused 😁
YorikSar
force-pushed
the
remove-extra-merge-in-meta
branch
2 times, most recently
from
803adcd to
9b7148e
Compare
Contributor
Author
|
The version with
All time metrics are worse, and we trade ~1 MiB of attrsets for ~0.5 MiB of lists. |
YorikSar
force-pushed
the
remove-extra-merge-in-meta
branch
from
9b7148e to
b679ba6
Compare
Contributor
Author
|
With null attr names trick metrics seem even better, thanks @philiptaron! Everything went down:
|
philiptaron
requested changes
Aug 2, 2025
Contributor
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
One trivial change then let's merge.
pkgs/stdenv/generic/check-meta.nix
Outdated
Contributor
There was a problem hiding this comment.
Will be good to merge once this is removed again as it is unused 😁
Remove usage of filterAttrs and two more attrset merges. Saves more time and memory.
YorikSar
force-pushed
the
remove-extra-merge-in-meta
branch
from
b679ba6 to
60d2cc0
Compare
Contributor
Author
|
@philiptaron Ah, it slipped through again. Removed. |
1 task
Contributor
Author
|
What if we do the same for mkDerivation?.. #430969 |
philiptaron
approved these changes
Aug 4, 2025
YorikSar
added a commit
to tweag/nixpkgs
that referenced
this pull request
Aug 7, 2025
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This operator merges two attrsets without any conditions, and leads to more operations and allocations.
Extract from the Eval / compare job summary:
All these metrics went down, all other metrics didn't change.
Especially note -2% of CPU and -3% of GC time, -2.7% of sets and -2.4% of update ops.
Things done
Add a 👍 reaction to pull requests you find important.