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] nvm_print_versions: re-implement using awk #2827

Merged
merged 1 commit into from Oct 13, 2022

Conversation

ryenus
Copy link
Contributor

@ryenus ryenus commented Jun 5, 2022

As of now there are about 700 node versions according to nvm_remote_versions, for which nvm 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:

BeforeAfter
Main loop real 17.163s user 6.635s sys 6.675s real 0.183s user 0.108s sys 0.098s
Overall real 18.555s user 6.984s sys 6.958s real 1.671s user 0.508s sys 0.414s

@ryenus
Copy link
Contributor Author

ryenus commented Jun 5, 2022

Tested using dash, both nvm ls and nvm ls-remote work as expected.
Interestingly that with this change nvm ls-remote is now faster than nvm ls.

reducing `nvm ls-remote` from almost 20s to below 2s.

Signed-off-by: ryenus <ryenus@gmail.com>
Copy link
Member

@ljharb ljharb left a 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 :-)

nvm.sh Show resolved Hide resolved
@ljharb ljharb added the performance This relates to anything regarding the speed of using nvm. label Jun 5, 2022
@ljharb
Copy link
Member

ljharb commented Jun 6, 2022

When i run this locally on my mac, i get:

awk: newline in string v0.1.14
v0.1.15
v0.1... at source line 1
awk: newline in string v0.1.14
v0.1.15
v0.1... at source line 1
awk: newline in string v0.1.14
v0.1.15
v0.1... at source line 1

are you using only features of posix awk? gnu-specific stuff won't be viable.

@ljharb ljharb marked this pull request as draft June 6, 2022 19:13
@ryenus
Copy link
Contributor Author

ryenus commented Jun 6, 2022

Indeed, I just pushed a change to substitute newlines with | when passed through awk -v. And now it works well with /usr/bin/awk on Mac.

@ryenus ryenus marked this pull request as ready for review June 6, 2022 23:50
@ryenus
Copy link
Contributor Author

ryenus commented Jun 7, 2022

BTW. about the failing tests on Travis, is it related to this comment?

@ljharb
Copy link
Member

ljharb commented Jun 7, 2022

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.

@ljharb
Copy link
Member

ljharb commented Jul 17, 2022

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.

@ryenus
Copy link
Contributor Author

ryenus commented Jul 18, 2022

I'm probably going to try to migrate off of travis soon, since we've been blocked for months.

Great, glad that we've moving forward, thank you!

@ljharb ljharb changed the title [perf] re-implement nvm_print_versions using awk [perf] nvm_print_versions: re-implement using awk Oct 8, 2022
@ljharb
Copy link
Member

ljharb commented Oct 8, 2022

@ryenus it looks like some tests are failing for real; could you take a look?

@ljharb ljharb marked this pull request as draft October 8, 2022 06:16
@ljharb
Copy link
Member

ljharb commented Oct 11, 2022

The tests pass now prior to this PR, so while i think you're right, i think a change here is still needed?

@ryenus
Copy link
Contributor Author

ryenus commented Oct 11, 2022

Sure, I'll have a deeper look once I get a chance.

@ljharb
Copy link
Member

ljharb commented Oct 11, 2022

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.

@ryenus ryenus force-pushed the master branch 3 times, most recently from 17e4d83 to 7caec24 Compare October 13, 2022 17:49
@ljharb ljharb marked this pull request as ready for review October 13, 2022 18:15
@ryenus
Copy link
Contributor Author

ryenus commented Oct 13, 2022

@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!

@tangkunyin
Copy link

When i run this locally on my mac, i get:

awk: newline in string v0.1.14
v0.1.15
v0.1... at source line 1
awk: newline in string v0.1.14
v0.1.15
v0.1... at source line 1
awk: newline in string v0.1.14
v0.1.15
v0.1... at source line 1

are you using only features of posix awk? gnu-specific stuff won't be viable.

So, did this be solved?

@ljharb
Copy link
Member

ljharb commented Nov 29, 2022

@tangkunyin yes, that's been fixed in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance This relates to anything regarding the speed of using nvm.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants