Add filterAttrs builtin #291
Conversation
WalkthroughAdds a new primitive Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/libexpr/primops.cc (1)
3508-3549: prim_filterAttrs implementation looks correct; consider a small perf refactorThe logic matches the usual
filterAttrspattern: you force the input attrset, short‑circuit the empty case, forcefonce, iterate attrs in sorted order, callf name value, and rebuild a sorted subset. That’s semantically in line with the nixpkgs/lib definition and other primops likefilter/mapAttrs.One minor performance improvement: you allocate a temporary
Value(vFun2) for each attribute just to partially applyfto the name. You can avoid that per‑attribute heap allocation by using the multi‑argumentcallFunctionoverload directly, as done infoldl':- for (auto & i : *args[1]->attrs()) { - Value * vName = Value::toPtr(state.symbols[i.name]); - Value * vFun2 = state.allocValue(); - vFun2->mkApp(args[0], vName); - Value res; - state.callFunction(*vFun2, *i.value, res, noPos); - if (state.forceBool(res, pos, "while evaluating the return value of the filtering function passed to builtins.filterAttrs")) - attrs.insert(i.name, i.value); - } + for (auto & i : *args[1]->attrs()) { + Value * vName = Value::toPtr(state.symbols[i.name]); + Value * vs[]{vName, i.value}; + Value res; + state.callFunction(*args[0], vs, res, noPos); + if (state.forceBool( + res, + pos, + "while evaluating the return value of the filtering function passed to builtins.filterAttrs")) + attrs.insert(i.name, i.value); + }Optionally, you could mirror
prim_filter’ssameoptimization (track whether any attrs were actually dropped and, if not, just return the original set) to avoid rebuilding when the predicate is always true, but that’s a non‑essential micro‑opt.src/libexpr-tests/primops.cc (1)
323-346: filterAttrs tests cover core behavior; optional extra case for name‑only predicatesThese tests correctly exercise the happy path (keeping only attributes with
value > 5) and the all‑false case, and they force the surviving values to ensure they evaluate as expected. That gives good coverage of the new primop.If you want slightly broader coverage, you could add an extra test where
fdepends only on the attribute name (e.g.name == "foo") to assert that the name argument is wired correctly and that values are only forced when the predicate chooses to use them, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libexpr-tests/primops.cc(1 hunks)src/libexpr/primops.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libexpr/primops.cc (2)
src/libexpr/include/nix/expr/attr-set.hh (4)
pos(399-404)pos(399-399)pos(406-411)pos(406-406)src/libexpr/include/nix/expr/eval.hh (24)
pos(712-713)pos(729-736)pos(759-759)pos(764-764)pos(769-773)pos(790-790)pos(904-904)pos(1042-1042)v(654-657)v(654-654)v(659-659)v(665-665)v(670-670)v(671-671)v(672-672)v(674-674)v(679-679)v(683-683)v(684-684)v(685-690)v(691-691)v(710-710)v(875-875)v(952-952)
src/libexpr-tests/primops.cc (2)
src/libexpr/eval.cc (20)
eval(1150-1153)eval(1150-1150)eval(1188-1191)eval(1188-1188)eval(1193-1196)eval(1193-1193)eval(1198-1201)eval(1198-1198)eval(1203-1206)eval(1203-1203)eval(1208-1211)eval(1208-1208)eval(1225-1320)eval(1225-1225)eval(1322-1344)eval(1322-1322)b(3291-3294)b(3291-3291)state(1084-1107)state(1084-1084)src/libexpr/include/nix/expr/attr-set.hh (2)
a(38-41)a(38-38)
edolstra
left a comment
edolstra
left a comment
There was a problem hiding this comment.
LGTM. Do you have any metrics on how much this helps with stuff like Nixpkgs/NixOS evaluation?
not-ronjinger
force-pushed
the
add-filterattrs-detsys
branch
2 times, most recently
from
d6ed2cc to
8962520
Compare
Here's a trivial example, showing roughly a 5x decrease in time and memory for just the filterAttrs call: I don't think |
|
Seems to be single digit % improvement for nixos eval. But it helps! With my changes (replaced Unmodified behavior (ran a second time to avoid writing .drv overhead) |
src/libexpr/primops.cc
Outdated
| vFun2->mkApp(args[0], vName); | ||
| Value res; | ||
| state.callFunction(*vFun2, *i.value, res, noPos); |
There was a problem hiding this comment.
Is this mkApp() needed? You can call callFunction() with multiple arguments, which would save an allocation.
There was a problem hiding this comment.
No, it's not. Wasn't aware, I'll change.
There was a problem hiding this comment.
changed to just passing both. No real effect on performance:
$ NIX_SHOW_STATS=1 /nix/store/b4qdkw8wdlfzm3gagrs5d8j7abh93xhj-determinate-nix-3.14.0/bin/nix-instantiate --eval -E 'with import ./. { allowAliases = false; }; lib.filterAttrs (_: v: v == "directory") (builtins.readDir ./pkgs/development/python-modules)' > /dev/null
{
"cpuTime": 0.09584499895572662,
"envs": {
"bytes": 2772064,
"elements": 190142,
"number": 156366
},
"gc": {
"cycles": 1,
"heapSize": 4295229440,
"totalBytes": 37539568
},
"list": {
"bytes": 373904,
"concats": 1222,
"elements": 46738
},
"maxWaiting": 0,
"nrAvoided": 142030,
"nrExprs": 98670,
"nrFunctionCalls": 147065,
"nrLookups": 74178,
"nrOpUpdateValuesCopied": 1153522,
"nrOpUpdates": 5024,
"nrPrimOpCalls": 59705,
"nrSpuriousWakeups": 0,
"nrThunks": 161819,
"nrThunksAwaited": 0,
"nrThunksAwaitedSlow": 0,
"sets": {
"bytes": 24388864,
"elements": 1504024,
"number": 13520
},
"sizes": {
"Attr": 16,
"Bindings": 24,
"Env": 8,
"Value": 16
},
"symbols": {
"bytes": 1485792,
"number": 43403
},
"time": {
"cpu": 0.09584499895572662,
"gc": 0.001,
"gcFraction": 0.010433512555641289
},
"values": {
"bytes": 8695104,
"number": 543444
},
"waitingTime": 0.0
}
a627178 to
d6e05fd
Compare
Head branch was pushed to by a user without write access
not-ronjinger
force-pushed
the
add-filterattrs-detsys
branch
from
d6e05fd to
9c96d74
Compare
|
Fixed the test linting |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/libexpr/primops.cc (1)
3508-3549: Consider a “no-op” fast-path + better error position for predicate calls. This primop always builds a new attrset; if the predicate keeps all attrs, returning the original set avoids allocations and preserves sharing. Also,callFunction(..., noPos)can make predicate errors harder to locate.static void prim_filterAttrs(EvalState & state, const PosIdx pos, Value ** args, Value & v) { state.forceAttrs(*args[1], pos, "while evaluating the second argument passed to builtins.filterAttrs"); if (args[1]->attrs()->empty()) { v = *args[1]; return; } state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filterAttrs"); auto attrs = state.buildBindings(args[1]->attrs()->size()); + bool same = true; for (auto & i : *args[1]->attrs()) { Value * vName = Value::toPtr(state.symbols[i.name]); Value * callArgs[] = {vName, i.value}; Value res; - state.callFunction(*args[0], callArgs, res, noPos); + state.callFunction(*args[0], callArgs, res, pos /* or i.pos if preferred/available */); if (state.forceBool( res, pos, "while evaluating the return value of the filtering function passed to builtins.filterAttrs")) attrs.insert(i.name, i.value); + else + same = false; } - v.mkAttrs(attrs.alreadySorted()); + if (same) + v = *args[1]; + else + v.mkAttrs(attrs.alreadySorted()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/libexpr/primops.cc(1 hunks)tests/functional/lang/eval-okay-filterattrs-names.exp(1 hunks)tests/functional/lang/eval-okay-filterattrs-names.nix(1 hunks)tests/functional/lang/eval-okay-filterattrs.exp(1 hunks)tests/functional/lang/eval-okay-filterattrs.nix(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/functional/lang/eval-okay-filterattrs.exp
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/functional/lang/eval-okay-filterattrs-names.exp
- tests/functional/lang/eval-okay-filterattrs.nix
🧰 Additional context used
🧬 Code graph analysis (1)
src/libexpr/primops.cc (2)
src/libexpr/include/nix/expr/attr-set.hh (4)
pos(399-404)pos(399-399)pos(406-411)pos(406-406)src/libexpr/include/nix/expr/eval.hh (24)
pos(712-713)pos(729-736)pos(759-759)pos(764-764)pos(769-773)pos(790-790)pos(904-904)pos(1042-1042)v(654-657)v(654-654)v(659-659)v(665-665)v(670-670)v(671-671)v(672-672)v(674-674)v(679-679)v(683-683)v(684-684)v(685-690)v(691-691)v(710-710)v(875-875)v(952-952)
🔇 Additional comments (1)
tests/functional/lang/eval-okay-filterattrs-names.nix (1)
1-5: Nice minimal name-based test forbuiltins.filterAttrs. Covers the 2-arg predicate shape and verifies name-driven filtering.
3 participants
Motivation
Similar to NixOS#14753, but rebased for determinate nix.
The implementation in nixpkgs is slightly inefficient:
Namely, it has to create a temporary list of names attrNames set, then has to scan through the entries with filter, apply the predicate which needs to dereference the value, then pass it to removeAttrs, which then needs to do another scan of names which failed the predicate. It's likely as good as you can get without creating a specific builtin.
With a native builtin, we should be able to eliminate the need for generating the temporary list, the initial scan should be a bit faster as there is less indirection with looking up the value, and the need to do another scan for removeAttrs should be eliminated altogether.
Re-introducing this to nixpkgs/lib should be easy with a builtins ? filterAttrs check.
Context
Gotta go fast 🚀
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.