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

[BUG] process.report.getReport() is very very slow on win32 #95

Closed
1 task done
wraithgar opened this issue Mar 18, 2024 · 5 comments · Fixed by #118
Closed
1 task done

[BUG] process.report.getReport() is very very slow on win32 #95

wraithgar opened this issue Mar 18, 2024 · 5 comments · Fixed by #118
Labels
Bug thing that needs fixing Priority 2 will get attention when we're freed up

Comments

@wraithgar
Copy link
Member

wraithgar commented Mar 18, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

According to @dstaley it can take tens of seconds for process.report.getReport to return on a win32 system. The results of this should be cached in some way that doesn't also require hacks to reset that cache during testing.

Expected Behavior

No response

Steps To Reproduce

No response

Environment

No response

@wraithgar wraithgar added Needs Triage needs an initial review Bug thing that needs fixing Priority 2 will get attention when we're freed up and removed Needs Triage needs an initial review labels Mar 18, 2024
@dstaley
Copy link

dstaley commented Mar 18, 2024

Just adding some additional context I've come across:

  • Slow execution time for getReport has been "fixed" in node with the addition of the report.excludeNetwork option. This should be safe to add since it's a noop on older versions of Node.
  • Other package managers (yarn and pnpm (via detect-libc)) check the file contents of /usr/bin/ldd for the presence of a glibc string as a shortcut, but that's hacky since the file contents can be literally whatever.

Also, just noting that even if this value is cached, users will still incur the cost of executing process.report.getReport() at least once, which can add upwards of 30s to installation time.

@wraithgar
Copy link
Member Author

I think caching is the right balance of doing things correctly, but as fast as possible inside those parameters. We will not be string pattern matching the ldd bin.

This is at heart a Node.js bug.

@wraithgar
Copy link
Member Author

Even if excludeNetwork made the call much faster this is still probably a good thing to cache since it's not expected to change inside a given runtime, and is called an unbounded number of times based on how many optional dependencies are in your tree that have a libc field.

@wraithgar wraithgar changed the title [BUG] process.report.getReport() is very very slow on wsl2 [BUG] process.report.getReport() is very very slow on win32 Mar 18, 2024
@wraithgar
Copy link
Member Author

An alternative that would technically work and be correct but is probably not the right path for npm itself would be to use node_gyp and expose a direct call to ctypes.CDLL, which is what node uses under the hood to get that information.

https://github.com/nodejs/node/blob/4ba9f45d991b3d0471bc20fcb1c234898b4aa1d7/tools/gyp/pylib/packaging/_manylinux.py#L117C29-L117C40

@wraithgar
Copy link
Member Author

report.excludeNetwork is probably the path of least resistance here. We should get that in sooner than later.

wraithgar added a commit that referenced this issue Sep 11, 2024
It is very slow in win32 environments and we don't need that info here

Closes: #95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Priority 2 will get attention when we're freed up
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants