Skip to content
This repository has been archived by the owner on May 4, 2024. It is now read-only.

perf: rewrite npmlog #178

Closed
wants to merge 2 commits into from
Closed

perf: rewrite npmlog #178

wants to merge 2 commits into from

Conversation

H4ad
Copy link

@H4ad H4ad commented Apr 6, 2024

This is a very risky PR since it could break a lot of things but I think it is worthy to have.

The loading graph before:
image

The loading graph after:
image

The loading time using hyperfine before:

$ hyperfine --warmup 3 "node ./lib/log.js"
Benchmark 1: node ./lib/log.js
  Time (mean ± σ):     114.9 ms ±   1.7 ms    [User: 101.4 ms, System: 37.9 ms]
  Range (min … max):   111.9 ms … 117.6 ms    25 runs

After:

$ hyperfine --warmup 3 "node ./lib/log.js"
Benchmark 1: node ./lib/log.js
  Time (mean ± σ):     104.1 ms ±   2.0 ms    [User: 91.6 ms, System: 34.4 ms]
  Range (min … max):    98.1 ms … 108.5 ms    29 runs

I also added another method called trackerRemoveAllListeners that can be used to never load the are-we-there-yet at https://github.com/npm/cli/blob/latest/lib/utils/display.js#L114

I think we should add more tests to gauge and also for tracker that didn't include mocked versions since I didn't even declare tracker and gauge during the tests and it passed all the tests because they are mocked.

@H4ad H4ad requested a review from a team as a code owner April 6, 2024 15:07
@lukekarrys
Copy link
Contributor

This is definitely a good idea! However I have a WIP PR that will remove npmlog and are-we-there-yet and gauge. It will add one new dependency proggy. Currently tests are not passing in that PR but as it gets close I would be interested in you taking a look at it for performance before it lands.

@H4ad
Copy link
Author

H4ad commented Apr 6, 2024

Send me the branch, I can take a look and see if it will worth continuing the refactoring (by looking at the load time)

@lukekarrys
Copy link
Contributor

npm/cli#7339

@lukekarrys
Copy link
Contributor

The branch might even be completely broken at the moment. It's a WIP that I'm hoping to finish next week.

@H4ad
Copy link
Author

H4ad commented Apr 6, 2024

Since you are working to remove npmlog, and the alternative looks faster, should I close this PR? I think you will deprecate this module, right?

@wraithgar
Copy link
Member

Yes we will deprecate this module. PLEASE be involved as much as you want in the rewrite efforts. This is our overview issue where we're tracking and discussing the efforts: npm/statusboard#810

@lukekarrys lukekarrys closed this Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants