add a sublist primop #2459
add a sublist primop #2459
Conversation
for one of the expressions ive been optimizing, it reduces: calls to elemAt from 108 million to 59 thousand adds 106k calls to builtins.sublist values created down from 400m to 5m total eval time down from 149sec to 15sec
|
LGTM! 👍 |
| int start = state.forceInt(*args[0], pos); | ||
| size_t len = state.forceInt(*args[1], pos); | ||
| state.forceList(*args[2], pos); | ||
| size_t count = std::min(args[2]->listSize() - start, len); |
There was a problem hiding this comment.
I think this underflows if start > listSize().
There was a problem hiding this comment.
i can confirm it malfunctions:
nix-repl> builtins.sublist 11 3 [ 1 2 3 4 5 6 7 8 9 10 ]
start is 11
len is 3
listSize is 10
count is 3
[ Segmentation fault (core dumped)
will update the PR soon
|
If you would, please also add an under flow test to the mix tests.
… On Oct 2, 2018, at 9:51 AM, Michael Bishop ***@***.***> wrote:
@cleverca22 commented on this pull request.
In src/libexpr/primops.cc:
> @@ -1490,6 +1490,18 @@ static void prim_concatLists(EvalState & state, const Pos & pos, Value * * args,
state.concatLists(v, args[0]->listSize(), args[0]->listElems(), pos);
}
+/* return a subsection of a list */
+static void prim_sublist(EvalState & state, const Pos & pos, Value * * args, Value & v)
+{
+ int start = state.forceInt(*args[0], pos);
+ size_t len = state.forceInt(*args[1], pos);
+ state.forceList(*args[2], pos);
+ size_t count = std::min(args[2]->listSize() - start, len);
i can confirm it malfunctions:
nix-repl> builtins.sublist 11 3 [ 1 2 3 4 5 6 7 8 9 10 ]
start is 11
len is 3
listSize is 10
count is 3
[ Segmentation fault (core dumped)
will update the PR soon
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Friendly ping! No rush on my account, just want to help ensure doesn't fall through the cracks :). |
|
I marked this as stale due to inactivity. → More info |
|
still need to get around to this, not completely stale |
|
I marked this as stale due to inactivity. → More info |
fricklerhandwerk
added
feature
|
Marked as draft because:
I think the idea is good, however. @cleverca22's numbers at the top are, well, shocking! And we have |
|
Triaged in Nix team meeting:
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-04-28-nix-team-meeting-minutes-50/27698/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-48/28102/1 |
|
Closing since #8290 took over (and is also closed by lack of evidence of the performance gain). |
for one of the expressions ive been optimizing, it reduces:
calls to elemAt from 108 million to 59 thousand
adds 106k calls to builtins.sublist
values created down from 400m to 5m
total eval time down from 149sec to 15sec
for a related expression in the same set, it reduces the eval time from 45 minutes to 2 minutes, while still producing the same
.drvpath at the enda 3rd related expression took over 147 minutes and 66gig of ram (and never finished, i had to kill it)
after this change, it took 19 minutes