Skip to content
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

Jump to bottom

Performance improvements #59756

Closed
wants to merge 5 commits into from
Closed

Performance improvements #59756

wants to merge 5 commits into from

Conversation

matthewbauer
Copy link
Member

Experiments in improving derivation performance. Trying two things:

  • inlining isCross
  • don't check hardening flags up front
Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@matthewbauer
make-derivation: inline isCross
b08de87 
Make isCross = stdenv.hostPlatform != stdenv.buildPlatform

This might be a little bit cheaper than redoing the comparison each
time.
@matthewbauer
make-derivation: don’t check hardening for each derivation
011d5fd 
This check could be expensive as it needs to perform each time before
the derivation can be evaluated.
@matthewbauer
Copy link
Member Author

@GrahamcOfBorg eval

@matthewbauer matthewbauer marked this pull request as ready for review April 24, 2019 03:04
@grahamc
Copy link
Member

grahamc commented Apr 24, 2019

Tomorrow I'll generate some flame graphs for comparison, but according to the performance report in the Checks tab, this is very promising. Some highlights:

  • function calls are reduced by ~1 million,
  • 83 mb cut off the heap
  • 4.5 million fewer lookups

Nicely done!

@offlinehacker
Copy link
Contributor

offlinehacker commented Apr 24, 2019 •
edited

Nice job 🙂 I have a question, how do you profile nix and what do you to create flame graphs?

@grahamc
Copy link
Member

grahamc commented Apr 24, 2019

It looks like #59369 isn't included in your branch, so it is a bit obscured by the unique function's interesting behavior. At any rate, here we are:

before: http://gsc.io/log.matthewbauer.pre.folded.svg

after: http://gsc.io/log.matthewbauer.folded.svg

@grahamc
Copy link
Member

grahamc commented Apr 24, 2019

https://github.com/NixOS/nixpkgs/pull/59756/checks?check_run_id=109424120 this report is generated by comparing the results from NIX_SHOW_STATS=1 nix-instantiate pkgs/top-level/all-packages.nix -A unstable.

For flame graphs: have like 30gb of ram (swap is ok but slow) and follow NixOS/nix#2782

@matthewbauer matthewbauer reopened this May 20, 2019
@matthewbauer matthewbauer changed the title [wip] Performance experiments Performance improvements Jun 6, 2019
@matthewbauer
Revert "make-derivation: inline isCross"
a1dcb88 
This reverts commit b08de87.

need to get the delta on this change
@matthewbauer
Copy link
Member Author

matthewbauer commented Jun 18, 2019 •
edited

@grahamc A question for you on the performance report:

Is it comparing performance results against the previous commit or the master branch? For instance I am trying to figure out the difference between these two results:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc Awaiting requested review from grahamc

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

Assignees
No one assigned
Labels
6.topic: stdenv Standard environment 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@matthewbauer @grahamc @offlinehacker @GrahamcOfBorg