-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
optimize primops and utils by caching more and copying less #5906
Conversation
Just for fun, I 'benchmarked' memory usage as measured by valgrind with
Assuming I did everything right, and I'm not confounding anything, that looks like a nice 3%-ish improvement :) If anyone wants to try to reproduce, my nixpkgs clone is checked out to the old-ish a523706d02434f38c36d2f20cfdd972074ce8e1e. |
there's a couple places that can be easily converted from using strings to using string_views instead. gives a slight (~1%) boost to system eval. # before nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 2.946 s ± 0.026 s [User: 2.655 s, System: 0.209 s] Range (min … max): 2.905 s … 2.995 s 20 runs # after Time (mean ± σ): 2.928 s ± 0.024 s [User: 2.638 s, System: 0.211 s] Range (min … max): 2.893 s … 2.970 s 20 runs
there's a few symbols in primops we can create once and pick them out of EvalState afterwards instead of creating them every time we need them. this gives almost 1% speedup to an uncached nix search.
symbols can also be cast to string_view, which compares the same but doesn't require a copy of both symbol names on every comparison.
the temporary will be discarded anyway, so we can move out of it and save many small allocations and copies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Just one suggestion to improve the maintainability/performance balance.
Also: If during the development you encountered any bug that feels like it should have been caught by the libutil testsuite, feel free to extend it, that could be very useful :)
the only "bug" we've found is that util's |
use a sorted array of symbols to be removed instead of a set. this saves a lot of memory allocations and slightly speeds up removal.
gives about 1% improvement on system eval, a bit less on nix search. # before nix search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 7.419 s ± 0.045 s [User: 6.362 s, System: 0.794 s] Range (min … max): 7.335 s … 7.517 s 20 runs nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 2.921 s ± 0.023 s [User: 2.626 s, System: 0.210 s] Range (min … max): 2.883 s … 2.957 s 20 runs # after nix search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 7.370 s ± 0.059 s [User: 6.333 s, System: 0.791 s] Range (min … max): 7.286 s … 7.541 s 20 runs nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 2.891 s ± 0.033 s [User: 2.606 s, System: 0.210 s] Range (min … max): 2.823 s … 2.958 s 20 runs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Giving it a few days before merging in case @edolstra wants to have a second look
We explicitly hack around to remove them, so might as well check that the hack is useful. (Introduced because I feared that the changes of #5906 (comment) would bring back some runtime references)
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/tweag-nix-dev-update-24/17230/1 |
avoiding string copies in in places where string views are sufficient and using cached symbols for constant keys used by primops improves uncached nix search and nixos system eval by about 2%. there's a lot more in utils and the evaluator that can be made to use string views for copy avoidance, but that'll be a separate PR because those changes are based on #5812 (while this one does not).