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

Improve startup performance. Closes #703 #705

Merged
merged 2 commits into from
Apr 2, 2015
Merged

Conversation

joliss
Copy link
Contributor

@joliss joliss commented Mar 27, 2015

No description provided.

@joliss
Copy link
Contributor Author

joliss commented Mar 27, 2015

I added a comment re nvm_ensure_version_installed performance in a separate commit; feel free to cherry-pick if you don't want it :)

@@ -1718,7 +1724,7 @@ if nvm_supports_source_options && [ "_$1" = "_--install" ]; then
elif nvm_rc_version >/dev/null 2>&1; then
nvm install >/dev/null
fi
elif nvm ls default >/dev/null; then
elif nvm_alias default >/dev/null 2>&1; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some of the tests failed :-/ I suspect this line is the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure; I'm having some trouble with the test suite (#708).

Edit: I think the line might be OK actually.

@ljharb
Copy link
Member

ljharb commented Apr 1, 2015

@joliss ping :-)

"_default")
# Special case for performance, as this is run on every shell startup
VERSION="$(nvm_alias default 2> /dev/null)"
;;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually turns out not to work correctly when the default alias is itself a symbol like "iojs" that needs to be resolved further.

@joliss
Copy link
Contributor Author

joliss commented Apr 2, 2015

Okay, Travis passes now. I had to remove some stuff; see my comment on outdated diff above. I think it's good to merge!

The other issues probably require someone to dig a bit deeper and fix the performance issues properly; I wrote up some findings at #709, but don't want to try and fix things at this point, as I don't know the code well enough.

ljharb added a commit that referenced this pull request Apr 2, 2015
Improve startup performance. Closes #703
@ljharb ljharb merged commit 3401d15 into nvm-sh:master Apr 2, 2015
@ljharb
Copy link
Member

ljharb commented Apr 2, 2015

Thanks! I'll take a look at #709 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants