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.

Sign up for GitHub

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

Jump to bottom

[Refactor] Reduce one more sed & pipe in nvm ls to speedup #1462

Merged
merged 1 commit into from
Jan 21, 2019
Merged

[Refactor] Reduce one more sed & pipe in nvm ls to speedup #1462

merged 1 commit into from
Jan 21, 2019

Conversation

PeterDaveHello
Copy link
Collaborator

Please note that this is similar but not the same as #1442, since there was a sort between sed so I need more time to confirm if there will be a problem in the refactor.

ljharb
ljharb reviewed Mar 28, 2017
View reviewed changes
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.

Indeed, the reason I didn't combine these before is that I need the sorting before the replacement.

@PeterDaveHello
Copy link
Collaborator Author

So is there a problem to sed before sort? If so, I'd like to take a look.

@ljharb
Copy link
Member

ljharb commented Mar 29, 2017

Since this code isn't very well tested, I remember testing it locally extensively when I updated it; and I intentionally didn't combine these last two sed commands. Please feel free to play with it locally and confirm.

@PeterDaveHello PeterDaveHello changed the title [Refactor] Reduce one more sed & pipe to speedup [WIP] [Refactor] Reduce one more sed & pipe to speedup Mar 29, 2017
@PeterDaveHello
Copy link
Collaborator Author

will do

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Apr 18, 2018 •
edited
Loading

Tested nvm ls output with these versions installed:

        v0.10.4 
       v0.10.48 
         v4.8.0
         v4.8.1
         v4.8.7
         v6.1.0
         v6.2.0
        v6.12.0
         v8.9.4
        v9.11.1

The results didn't change

        v0.10.4
       v0.10.48
         v4.8.0
         v4.8.1
         v4.8.7
         v6.1.0
         v6.2.0
        v6.12.0
         v8.9.4
        v9.11.1
default -> 8 (-> v8.9.4)
node -> stable (-> v9.11.1) (default)
stable -> 9.11 (-> v9.11.1) (default)
iojs -> N/A (default)
lts/* -> lts/carbon (-> N/A)
lts/argon -> v4.9.1 (-> N/A)
lts/boron -> v6.14.1 (-> N/A)
lts/carbon -> v8.11.1 (-> N/A)

I assmue the change will be catched by tests as the change in #1517 changed the output ordering before and got caught by the tests?

I'll fix the conflicts first anyway.

@PeterDaveHello PeterDaveHello changed the title [WIP] [Refactor] Reduce one more sed & pipe to speedup [Refactor] Reduce one more sed & pipe in nvm ls to speedup Apr 18, 2018
@PeterDaveHello
Copy link
Collaborator Author

@ljharb I test output results of hundreds of nvm ls again and see no difference yet, anything I can do here?

@ljharb ljharb force-pushed the sed-improve branch 2 times, most recently from b731073 to e367533 Compare January 20, 2019 23:46
ljharb
ljharb approved these changes Jan 20, 2019
View reviewed changes
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.

Let's give it a shot.

@ljharb ljharb merged commit e367533 into nvm-sh:master Jan 21, 2019
@ljharb ljharb added the performance This relates to anything regarding the speed of using nvm. label Jan 21, 2019
@PeterDaveHello PeterDaveHello deleted the sed-improve branch January 21, 2019 10:01
@PeterDaveHello
Copy link
Collaborator Author

Thanks.

@ljharb
Copy link
Member

ljharb commented Jan 22, 2019

@PeterDaveHello ok, well, figured out what this breaks. This is my nvm ls:

    iojs-v3.3.1
    iojs-v2.5.0
        v0.6.21
        v0.7.12
    iojs-v1.8.4
        v0.8.28
        v0.9.12
       v0.10.48
       v0.11.16
       v0.12.18
         v4.9.1
        v5.12.0
        v6.14.4
        v6.16.0
        v7.10.1
         v8.6.0
         v8.9.1
        v8.10.0
        v8.11.3
        v8.11.4
        v8.12.0
        v8.13.0
        v8.14.1
        v8.15.0
        v9.11.2
        v10.0.0
        v10.1.0
        v10.2.1
        v10.3.0
        v10.4.1
        v10.5.0
        v10.6.0
        v10.7.0
        v10.8.0
        v10.9.0
       v10.10.0
       v10.11.0
       v10.12.0
       v10.13.0
       v10.14.2
       v10.15.0
        v11.0.0
        v11.1.0
        v11.2.0
        v11.3.0
        v11.4.0
        v11.5.0
        v11.6.0
->      v11.7.0

In other words, you may not have accounted for io.js versions.

I'm going to have to revert this, unless you think you can put up a fix ASAP?

@PeterDaveHello
Copy link
Collaborator Author

Let me take a look.

@PeterDaveHello PeterDaveHello mentioned this pull request Jan 22, 2019
Closed
ljharb added a commit that referenced this pull request Feb 24, 2019
@ljharb
[Fix] nvm_ls: Reverts #1462 to fix io.js sorting.
2a513a1 
<details>
<summary>Before this revert:</summary>

```sh
iojs-v3.3.1
iojs-v2.5.0
v0.6.21
v0.7.12
iojs-v1.8.4
v0.8.28
v0.9.12
v0.10.48
v0.11.16
v0.12.9
v0.12.18
v0.12.87
v4.9.1
v5.11.1
v5.12.0
v6.14.4
v6.16.0
v7.10.1
v8.6.0
v8.9.1
v8.10.0
v8.11.3
v8.11.4
v8.12.0
v8.13.0
v8.14.1
v8.15.0
v9.11.2
v10.0.0
v10.1.0
v10.2.1
v10.3.0
v10.4.1
v10.5.0
v10.6.0
v10.7.0
v10.8.0
v10.9.0
v10.10.0
v10.11.0
v10.12.0
v10.13.0
v10.14.2
v10.15.1
v11.0.0
v11.1.0
v11.2.0
v11.3.0
v11.4.0
v11.5.0
v11.6.0
v11.7.0
v11.8.0
v11.9.0
v11.10.0
```
</details>

<details>
<summary>After this revert:</summary>

```sh
v0.6.21
v0.7.12
v0.8.28
v0.9.12
v0.10.48
v0.11.16
v0.12.9
v0.12.18
v0.12.87
iojs-v1.8.4
iojs-v2.5.0
iojs-v3.3.1
v4.9.1
v5.11.1
v5.12.0
v6.14.4
v6.16.0
v7.10.1
v8.6.0
v8.9.1
v8.10.0
v8.11.3
v8.11.4
v8.12.0
v8.13.0
v8.14.1
v8.15.0
v9.11.2
v10.0.0
v10.1.0
v10.2.1
v10.3.0
v10.4.1
v10.5.0
v10.6.0
v10.7.0
v10.8.0
v10.9.0
v10.10.0
v10.11.0
v10.12.0
v10.13.0
v10.14.2
v10.15.1
v11.0.0
v11.1.0
v11.2.0
v11.3.0
v11.4.0
v11.5.0
v11.6.0
v11.7.0
v11.8.0
v11.9.0
v11.10.0
```
</details>

In other words, the sorting needs to happen *before* the `NVM_NODE_PREFIX` is removed.
@ljharb
Copy link
Member

ljharb commented Feb 24, 2019

@PeterDaveHello I've reverted this PR in 2a513a1 due to #1462 (comment) ; feel free to make a new PR if you can get it passing.

@PeterDaveHello
Copy link
Collaborator Author

okay, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
performance This relates to anything regarding the speed of using nvm.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PeterDaveHello @ljharb