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: checkPlatform takes an insane amount of time to complete when used in a loop #120

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Tofandel
Copy link

@Tofandel Tofandel commented Oct 28, 2024

Yes this is a bit dirty because the report is now generated once at the beginning of a command which could be considered a side effect (but normally treeshaking will only include it only if needed), but the whole process.report is a weirdly built API meant for debugging and not really meant to be used in a normal program

But somehow when the call is done in the beginning of the process, this call is very fast, but when it's called after having run a http request (as is currently the case, a single call takes a lot more time to complete, it takes 40s or more on my system when called at the end of npm upgrade) but only a few milliseconds when called at the beginning (this is why it's best to run it outside the function at the beginning of the process as a side effect instead of calling getReport on demand and cache the result)

Here is a log of console.time('report') and console.timeEnd('report') before and after the getReport call
when run in the end of the upgrade command:
⠼report: 3:10.573 (m:ss.mmm)
when run in the beginning of the process at the top level
report: 1.943ms

This fixes npm hanging npm/cli#4028, npm/cli#7814, npm/cli#7868

@Tofandel Tofandel requested a review from a team as a code owner October 28, 2024 16:29
@Tofandel Tofandel changed the title Fix checkPlatform takes an insane amount of time to complete when used in a loop fix: checkPlatform takes an insane amount of time to complete when used in a loop Oct 28, 2024
@Tofandel
Copy link
Author

Another option is to copy the logic from yarn which also fixed this issue by looking into the /usr/bin/ldd file and fallback to getReport after

https://github.com/yarnpkg/berry/blob/f59bbf9f3828865c14b06a3e5cc3ae284a0db78d/packages/yarnpkg-core/sources/nodeUtils.ts#L28-L67

@IngFu
Copy link

IngFu commented Nov 1, 2024

Another option is to copy the logic from yarn which also fixed this issue by looking into the /usr/bin/ldd file and fallback to getReport after

https://github.com/yarnpkg/berry/blob/f59bbf9f3828865c14b06a3e5cc3ae284a0db78d/packages/yarnpkg-core/sources/nodeUtils.ts#L28-L67

Is getReport() called once during npm install or multiple times?

@Tofandel
Copy link
Author

Tofandel commented Nov 1, 2024

Currently multiple times because it's called at every checkPlatform, which is done in a loop once per installed package during npm install, which is what this PR is attempting to fix. I have also submitted a fix to the node js implementation so that get Report doesn't take long times if excludeNetwork is true, at which point we don't need to cache getReport early, just caching it after the first call will be enough

@IngFu
Copy link

IngFu commented Nov 1, 2024

Many projects have more than 100 dependencies. In my opinion it makes sense to limit the getReport call to one time as it performs other tasks as well and the system properties wont chance during installation.

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.

2 participants