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

lib: add concatMapAttrsToList #369146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

9ary
Copy link
Contributor

@9ary 9ary commented Dec 29, 2024

This is the concatMap counterpart to mapAttrsToList.

Usage example: nix-community/home-manager#6229

cc @bb010g

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Dec 29, 2024
This is the concatMap counterpart to mapAttrsToList.

Co-authored-by: Dusk Banks <[email protected]>
Comment on lines +1114 to +1115
# FIXME
concatMapAttrsToList :: (String -> a -> b) -> AttrSet -> [b]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am very confused by the type syntax, I'll need help to get this right (this is currently a copy-paste of mapAttrsToList).

Copy link
Member

Choose a reason for hiding this comment

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

This, right?

Suggested change
# FIXME
concatMapAttrsToList :: (String -> a -> b) -> AttrSet -> [b]
# FIXME
concatMapAttrsToList :: (String -> a -> [b]) -> AttrSet -> [b]


:::
*/
concatMapAttrsToList =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming is hard. concatMapAttrs is taken by what should be mergeMapAttrs. I've also considered mapAttrsToConcatList.

Copy link
Member

@rhendric rhendric left a comment

Choose a reason for hiding this comment

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

So this is, I think, f: attrs: concatLists (attrValues (mapAttrs f attrs))?

Should we be aiming for giving a name to all combinations of builtins primitives like this below some size? If not, what's the argument for why this combination gets special treatment? (I'm not presupposing that there isn't one; I'd just like to understand what you have in mind.)

Comment on lines +1114 to +1115
# FIXME
concatMapAttrsToList :: (String -> a -> b) -> AttrSet -> [b]
Copy link
Member

Choose a reason for hiding this comment

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

This, right?

Suggested change
# FIXME
concatMapAttrsToList :: (String -> a -> b) -> AttrSet -> [b]
# FIXME
concatMapAttrsToList :: (String -> a -> [b]) -> AttrSet -> [b]

@9ary
Copy link
Contributor Author

9ary commented Jan 4, 2025

So this is, I think, f: attrs: concatLists (attrValues (mapAttrs f attrs))?

I am completely unfamiliar with the underlying implementation, but I think this would allocate an extra attrset and a list? If so, this defeats the point here, see below.
Otherwise, mapAttrsToList could also be f: attrs: attrValues (mapAttrs f attrs), but it isn't.

Should we be aiming for giving a name to all combinations of builtins primitives like this below some size? If not, what's the argument for why this combination gets special treatment? (I'm not presupposing that there isn't one; I'd just like to understand what you have in mind.)

So the rationale is efficient map-filter over an attrset into a list (it's probably useful for other things, but that's the main use-case). This takes advantage of empty and one-element lists being stored efficiently, while filterAttrs before or filter after mapAttrsToList costs an extra allocation. This causes comparable gc churn to just mapAttrsToList.
This construct was suggested by @bb010g so I'll defer to her for more technical details.

I'd love to call this mapFilterAttrsToList if it was possible to expose an interface that matches the name, but nix doesn't have an option type.
Either way, I now see that amending the doc comment to explain the primary use-case would be valuable.

@rhendric
Copy link
Member

rhendric commented Jan 5, 2025

So the rationale is efficient map-filter over an attrset into a list (it's probably useful for other things, but that's the main use-case).

Efficiency, okay. Yes, this does save an allocation relative to using lib.mapAttrsToList with concatLists.

Do you expect to use this somewhere that is called enough times or on a large enough attrset for that to make any sort of noticeable difference? There are a few places in Nixpkgs where the concatLists (mapAttrsToList ...) combo could be replaced with this; I tried replacing one that I thought would be most impactful (in nixos/lib/systemd-lib.nix, part of the code that generates the text of unit files) with your implementation and while it made some difference, we're talking hundredths of a percent in metrics targeting this change specifically. I'm not convinced that's a gain worth increasing the number of library functions new folks have to wade through.

(The number of bytes used by lists, in my benchmark of evaluating my local configuration, went down from 23,018,728 to 23,015,560. Count of concatenations went down from 439,757 to 439,513. Time elapsed for nix-instantiate '<nixpkgs/nixos>' -A system --eval, averaged over 100 runs with hyperfine, went from 6.883 s ±0.065 s to 6.892 s ±0.061 s—no observable change, in other words.)

mapAttrsToList is used in many more places than this extension of it would be, and so it has an easier time earning its place in the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants