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

fix(perf): avoid importing the entire semver package for update-notifier #7346

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Apr 6, 2024

There are a bunch of places were we load semver, I'm trying to see if I can remove the full import for semver and only import the specific functions.

Currently, I didn't have any perf improvement since we still load the entire semver, once we have removed all the package loads, then we could see some improvement (a little bit).

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

Update notifier is often skipped due to config or if it has been run recently. In addition to this, do you think we should lazy load all the requires in this file?

@H4ad H4ad force-pushed the perf/startup-time-update-notifier branch from 408a717 to c57b0be Compare April 6, 2024 16:56
@H4ad H4ad changed the title perf: avoid importing the entire semver package for update-notifier chore: avoid importing the entire semver package for update-notifier Apr 6, 2024
@H4ad
Copy link
Contributor Author

H4ad commented Apr 6, 2024

@lukekarrys, if you think it will not hurt the maintainability, lazy loading everything will save ~300us.

Most of the require is already in the cache, so the impact is very minimal of having them at the top of the file.

@wraithgar
Copy link
Member

The update notifier, as far as I can remember, is called in a completely asynchronous way that gracefully handles a situation where the npm command is done before it finishes. Optimizing it is probably not needed.

If you find a situation where the update notifier is blocking the event loop, causing npm to not exit if the command is already finished, that is an actionable situation. Otherwise it's probably not worth the effort.

@H4ad
Copy link
Contributor Author

H4ad commented Apr 6, 2024

is called in a completely asynchronous way

The require is synchronous, even if we do not wait the return value.

About this module, just refactoring the imports for semver currently is enough.

@lukekarrys
Copy link
Contributor

I also want to note that this may cause fluctuations in benchmarking. You can add --no-update-notifier to you benchmarking command to ensure these lazy requires are never called.

@lukekarrys lukekarrys changed the title chore: avoid importing the entire semver package for update-notifier fix(perf): avoid importing the entire semver package for update-notifier Apr 7, 2024
@wraithgar wraithgar merged commit 70497cb into npm:latest Apr 7, 2024
22 checks passed
@github-actions github-actions bot mentioned this pull request Apr 7, 2024
@H4ad H4ad deleted the perf/startup-time-update-notifier branch April 7, 2024 15:43
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.

3 participants