lib/modules: Short-circuit unmatchedDefns when configs is empty#144022
lib/modules: Short-circuit unmatchedDefns when configs is empty#144022infinisil merged 6 commits intoNixOS:masterfrom
Conversation
ofborg
bot
added
10.rebuild-darwin: 0
roberth
force-pushed
the
lib-modules-optimize-unmatchedDefns
branch
from
1aeaf7a to
75d67de
Compare
roberth
force-pushed
the
lib-modules-optimize-unmatchedDefns
branch
from
75d67de to
d6ebd53
Compare
|
@infinisil could you double-check? |
lib/modules.nix
Outdated
| # Propagate all unmatched definitions from nested option sets | ||
| mapAttrs (n: v: v.unmatchedDefns) resultsByName | ||
| # Plus the definitions for the current prefix that don't have a matching option | ||
| // removeAttrs defnsByName' (attrNames matchedOptions); |
There was a problem hiding this comment.
After taking a closer look, I think the main savings come from defnsByName' not being evaluated when configs == [], meaning that config sections aren't evaluated when not needed.
Since this does make module more lazy to some degree it would be awesome if we could have a test case that shows that ensures that extra lazy behavior so we can see the impact and prevent future regressions of it.
There was a problem hiding this comment.
I've tested this for options now, but not for config yet.
There was a problem hiding this comment.
I couldn't find a test case. The savings may come exclusively from generating fewer internal datastructures.
Doesn't seem to have been a problem actually, but now it won't regress.
In hot code, the overhead (envs, applies) can matter.
Very confusing otherwise.
recursiveUpdate does not produce an attrset until it has evaluated
both its arguments to weak head normal form.
nix-repl> lib.recursiveUpdate (throw "a") (throw "b")
error: b
nix-repl> lib.recursiveUpdate (throw "a") {}
error: a
|
I've spotted some low hanging fruit that I've added here because it's simple enough. Will make separate PRs for other improvements though, because ofborg stats aren't comparable as it merges with (mutable) master before each run. |
|
@infinisil That's all, as far as I'm concerned. There's potential for more laziness, allowing |
|
I haven't invested much time but apparently this broke the way |
|
@andir Are you sure it isn't fixed by #148315? I don't know about eadwu's #144022 (reference), but it doesn't seem to be part of any ref anymore. |
I think you are right. It must be that other PR. I somehow didn't find that. |
3 participants
Motivation for this change
Improve performance.
By short-circuiting, we don't have to evaluate as much of the
optionstrees.Allocation stats show -0.55% percent in heap size.
benchshowsBefore:
After:
suggesting a 1.6% performance improvement, although that number is still bogus, considering the possible range of each combined measurement. Maybe I should have picked a lower confidence interval. I was hoping it would produce more samples for a high ci. I might still try that.
Performance report for d6ebd53 https://github.com/NixOS/nixpkgs/pull/144022/checks?check_run_id=4062089224
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)