-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
chore: improve performance of nvm version command #1695
Conversation
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.
There's a few legit tests failing.
In particular, some of the substitutions you're using seem to not be working.
nvm.sh
Outdated
@@ -13,7 +13,7 @@ | |||
NVM_SCRIPT_SOURCE="$_" | |||
|
|||
nvm_echo() { | |||
command printf %s\\n "$*" 2>/dev/null | |||
echo "$*" 2>/dev/null |
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.
Why this change? This is problematic; the command
is necessary to bypass any user aliases or functions, and the printf
is more reliable than echo
across shells.
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.
I have described it in the description.
I have changed command printf on echo because printf is slower than echo
(printf - 2ms, echo - 0.09ms)
What do you think about command echo
?
Thanks
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.
We can certainly try it; I think i added automated tests around this one. Specifically the issue was about multiline input on specific shells, I think dash and sh.
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.
🤔 I have checked and I was wrong.
command printf
majorized by command
so if we could not remove command I just revert this change
Thanks
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.
Sounds good; some slowdowns are inevitable for correctness, sadly, but we can still try to shave off time wherever we can :-)
nvm.sh
Outdated
else | ||
nvm_echo "${VERSION}" | ||
fi | ||
VERSION=${${NVM_LS_CURRENT_NODE_PATH#$NVM_DIR/versions/node/}%/bin/node} |
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.
This is reporting "bad substitution" - https://travis-ci.org/creationix/nvm/jobs/320732790#L1029
Still seeing test failures :-/ |
@ljharb yup, tests fail because they rely on some externals commands for example that code will call |
I will try to figure out how fix tests |
@maksimr hi! it's been awhile, but i've rebased this and it's failing some tests. any chance you're still interested in finishing this PR? |
@ljharb I have checked changes again with Thanks! |
Ok, that’s unfortunate - any speed improvement helps. If you ever change your mind, please reopen this PR in the future. |
@ljharb sure, we use nvm heavily but I guess that main pitfall from the issue was fixed by node itself or by Moore law :) |
fix #1694
This pull request reduces CPU time from 50ms to 15ms.
What was done:
I have changedcommand printf
onecho
becauseprintf
is slower than echo(printf - 2ms, echo - 0.09ms)
chore: improve performance of nvm version command #1695 (comment)
nvm_tree_contains_path
on string comparison(while - 14ms, str comparison - 0.06ms)
nvm_ls_current
fromnode --version
to getting it fromNVM_LS_CURRENT_NODE_PATH
.(node --version - 13ms, from string - 0.09ms)
Before:
After: