Prototype: accumulate attrset updates, perform k-way merge #11290
Conversation
This file contains 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 seems to be slower as of yet.
TODO
- [ ] remove merge pass 2 into 1:
- pass 1: fill UpdateQueue
- pass 2: stats (like max output size)
- pass 3: k-way merge
- [ ] maybe tracking sortedness is useful and cheap? in pass 1, building the UpdateQueue
- [ ] make it actually fast; different algorithm depending on size distribution and number of update ops?
- currently solution is the min-heap or priority queue approach
- divide and conquer (see also Nixpkgs which manually implements this for pkgs/by-name)
binary tree-shaped sorted merge operations
- can we take the size of attrsets into account to balance the work?
1000 + (1 + (1 + 1)) is better than ((1000+1)+1)+1 (where number refers to attrset size)
- iterative pairwise merging is more or less what we had, the tree of updates is flattened. Probably worse.
- perhaps some combination of solutions, depending on a heuristic
- [ ] split callFunction into two parts, effectively
- call2Thunk (create the new scope's Env etc)
- eval that thunk (evaluate the body)
- [ ] implement ExprApply::evalForUpdate - easy after the split
- [ ] make foldl' (//) work?
- [ ] optimize listToAttrs and other primops?
roberth
commented
Aug 13, 2024
Comment on lines
+1901
to
+1902
| e1->evalForUpdate(state, env, q); | ||
| e2->evalForUpdate(state, env, q); |
There was a problem hiding this comment.
Suggested change
| e1->evalForUpdate(state, env, q); | |
| e2->evalForUpdate(state, env, q); | |
| evalForUpdate(state, env, q); |
Comment on lines
+2043
to
+2045
| eval(state, env, vTmp); | ||
| // TODO add pos and errorCtx params | ||
| state.forceAttrs(vTmp, noPos, "while evaluating an attribute set merge operand"); |
| return pos; | ||
| } | ||
|
|
||
| virtual void evalForUpdate(EvalState & state, Env & env, UpdateQueue & q) override; |
There was a problem hiding this comment.
This is the only new code in this declaration. The rest is just an expansion of the MakeBinOp macro.
There was a problem hiding this comment.
Should make a MakeBinOpMembers macro that MakeBinOp this can both uses?
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-08-12-nix-team-meeting-minutes-168/50561/1 |
roberth
changed the title
roberth
changed the title
roberth
added
language
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 seems to be slower as of yet. Main point was showing an implementation strategy for an optimization that applies also to other semigroups, like string concatenation and lists.
I didn't originally plan to pursue this, as mentioned, so if you're interested in algorithms and performance optimization you are more than welcome to work on this.
TODO
Context
Yesterday's meeting (will post notes in a minute)
@tomberek
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.