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

[Perf] use/nvm_die_on_prefix: replicate npm config algorithm and remove npm config call #2317

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Oct 9, 2020

Fixes #2251. Fixes #1774. Fixes #1694. Fixes #1277. Fixes #1261. Fixes #1242. Fixes #966. Fixes #926. Fixes #860. Fixes #781. Fixes #709. Closes #1826. Closes #730.

This PR removes the call to npm config get prefix on nvm use, instead partially replicating the algorithm npm itself uses to build up its config. Since we don't have to actually produce a config, merely detect if any option is set that would influence npm config get prefix (namely, prefix or globalconfig), the checks are simpler and can be done relatively quickly. Previously, #1679, 1458de7, 8ee6f30, and a1def71 implemented partial forms of this checking. This PR specifically adds the checks for npmrc files, which allows the call to npm to be removed.

@ljharb ljharb added the performance This relates to anything regarding the speed of using nvm. label Oct 9, 2020
@jleedev
Copy link

jleedev commented Oct 9, 2020

Here's the speedup I'm seeing with this change:

$ /usr/bin/time bash -c '. ~/pkg/ljharb_nvm/nvm.sh' 
0.29user 0.05system 0:00.34elapsed 98%CPU (0avgtext+0avgdata 4408maxresident)k
0inputs+8outputs (0major+3300minor)pagefaults 0swaps
$ /usr/bin/time bash -c '. ~/.nvm/nvm.sh' 
3.85user 1.27system 0:04.98elapsed 102%CPU (0avgtext+0avgdata 39664maxresident)k
0inputs+16outputs (0major+51708minor)pagefaults 0swaps

@ljharb
Copy link
Member Author

ljharb commented Oct 9, 2020

Wow, that seems huge - thank you for the benchmarks!

@ljharb ljharb added the hacktoberfest-accepted If you're interested in a free shirt, this PR counts towards it. label Oct 9, 2020
@ljharb ljharb force-pushed the npm_config branch 2 times, most recently from 93cc2ab to 4cfc3f9 Compare October 12, 2020 06:20
@ljharb ljharb marked this pull request as ready for review October 14, 2020 21:03
@Schniz
Copy link

Schniz commented Oct 27, 2020

Wow!

@reasonablytall
Copy link
Contributor

The current version of nvm doesn't run as long on my machine, but still quite an improvement! Running against nvm --version => 0.36.0

bash

$ hyperfine "bash -c '. ~/.nvm/nvm.sh'" "bash -c '. ~/proj/nvm/nvm.sh'"  
Benchmark #1: bash -c '. ~/.nvm/nvm.sh'
  Time (mean ± σ):     628.6 ms ±   7.8 ms    [User: 614.2 ms, System: 142.0 ms]
  Range (min … max):   619.3 ms … 641.8 ms    10 runs
 
Benchmark #2: bash -c '. ~/proj/nvm/nvm.sh'
  Time (mean ± σ):     346.4 ms ±   4.4 ms    [User: 319.1 ms, System: 112.4 ms]
  Range (min … max):   340.1 ms … 355.3 ms    10 runs

zsh

$ hyperfine "zsh -c '. ~/.nvm/nvm.sh'" "zsh -c '. ~/proj/nvm/nvm.sh'" 
Benchmark #1: zsh -c '. ~/.nvm/nvm.sh'
  Time (mean ± σ):     764.0 ms ±  11.7 ms    [User: 756.5 ms, System: 158.1 ms]
  Range (min … max):   745.8 ms … 780.0 ms    10 runs
 
Benchmark #2: zsh -c '. ~/proj/nvm/nvm.sh'
  Time (mean ± σ):     482.3 ms ±   6.8 ms    [User: 467.3 ms, System: 122.4 ms]
  Range (min … max):   475.5 ms … 496.4 ms    10 runs

I'm unsure where that >100ms between zsh and bash comes from though...

@MatthiasPortzel
Copy link

MatthiasPortzel commented Oct 30, 2020

Seems to be roughly twice as fast, from around 400ms to around 200ms. (But I'm getting quite a bit of variance.)

I would love one day for nvm to be optimized down to the 30ms that it takes to start with --no-use, since right now it is the only slow part of my shell startup. But I can confirm that the remaining 200ms are not a result of prefix checking.

@ljharb
Copy link
Member Author

ljharb commented Oct 31, 2020

@matthiassaihttam thanks! if you have any more information (performance metrics, which parts of nvm contribute how much to that extra 170ms, etc), filing a new issue with it would be greatly appreciated.

@ljharb ljharb mentioned this pull request Nov 5, 2020
@ljharb ljharb merged commit 499d303 into nvm-sh:master Nov 6, 2020
@ljharb ljharb deleted the npm_config branch November 6, 2020 18:11
@ljharb
Copy link
Member Author

ljharb commented Nov 6, 2020

v0.37.0 has been released with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted If you're interested in a free shirt, this PR counts towards it. performance This relates to anything regarding the speed of using nvm.
Projects
None yet
5 participants