-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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] nvm_print_versions
: re-implement using awk
#2827
Conversation
Tested using dash, both |
reducing `nvm ls-remote` from almost 20s to below 2s. Signed-off-by: ryenus <ryenus@gmail.com>
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.
Can't say i understand all the awk code, but passing tests and some local testing should be sufficient :-)
When i run this locally on my mac, i get:
are you using only features of posix awk? gnu-specific stuff won't be viable. |
Indeed, I just pushed a change to substitute newlines with |
BTW. about the failing tests on Travis, is it related to this comment? |
Works great, and wow, it really is lightning fast by comparison! Travis tests are sadly a blocker, but it's being worked on in #2802. |
I'm probably going to try to migrate off of travis soon, since we've been blocked for months. I agree the likelihood of this breaking is minimal, but it's not worth the risk to merge anything while tests aren't passing. |
Great, glad that we've moving forward, thank you! |
nvm_print_versions
: re-implement using awk
@ryenus it looks like some tests are failing for real; could you take a look? |
The tests pass now prior to this PR, so while i think you're right, i think a change here is still needed? |
Sure, I'll have a deeper look once I get a chance. |
The failure I see (https://app.travis-ci.com/github/nvm-sh/nvm/jobs/585309369#L566) is because the non-color output is missing the asterisks. |
17e4d83
to
7caec24
Compare
@ljharb, the patch is finally in a good shape, there's a last minute change I needed to include, meanwhile I saw your formatting tweaks, which is totally ok for me. With that said, over to you now, thanks! |
So, did this be solved? |
@tangkunyin yes, that's been fixed in the PR. |
As of now there are about 700 node versions according to
nvm_remote_versions
, for whichnvm ls-remote
might take up to 20 seconds to print the versions in a slightly formatted form.We can get a significant performance improvement by replacing the major part of
nvm_print_versions
with a awk based implementation:real 17.163s user 6.635s sys 6.675s
real 0.183s user 0.108s sys 0.098s
real 18.555s user 6.984s sys 6.958s
real 1.671s user 0.508s sys 0.414s