-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Add runtime information about libc version #48204
Comments
Why? Have you actually found this to be a major bottleneck somehow? |
Yes, see #46060. |
I don't know what you mean by major bottleneck but spending more than 5ms just to check if the system is
No, my intention is not to try to make
|
Also, napi-rs also checks musl when importing/running your code, this is the default emitted code from Napi, so improving this helps a lot NodeJS libraries with Rust bindings. |
If the scenario we're discussing here is downloading of precompiled binaries for node addons, I think the binary download, the related fs operations, and other parts of the process are going to dwarf calls to
Again, improving |
It depends on the implementation but I agree that this can't be a big issue if it only happens on installation, but it will depend on whether NPM accepts and adds support for
I'm discussing this topic on lovell/detect-libc#18 (comment) to understand better the implications, if it's okay to use always the fallback, for sure that should be used all the time. Anyway, 96k op/s is fine, but it could just be waay faster |
This doesn't necessarily happen only when downloading, it's also typically necessary to check the libc type each time before loading the addon. Only resolving it once when downloading and writing the addon file under a common name is often not practical (especially when distributing the addon binaries via npm; although in our case it's not possible for certain reasons even though the download logic is custom and the binaries are downloaded from S3). To clarify, in my case we're not currently using |
This isn't perfectly solvable because there is no reliable way to feature-detect musl libc. The approach that You could construe the absence of glibc-specific defines or symbols as an indicator of musl but obviously that's not foolproof, glibc and musl aren't the only linux libcs. Opening /proc/self/exe and scanning the ELF header for the dynamic linker section or shared objects section won't work when node is statically linked. Basically, every approach gets it right some of the time but never all the time. |
Good to know, I just assumed this was solvable at compile time, and without checking the code I also assumed that what
I feel like this might actually be the best solution in the context of loading native addons. If node is statically linked, then libc doesn't matter since dynamic loading is not supported anyway. But then again, it's possible to do this in userland and it doesn't necessarily have to be in core. |
That's true for musl but it sort of works with glibc (if you have libc.so around and don't need multi-namespace support.) |
I see, thanks for clarification. |
Currently, all the NodeJS (>10.x) applications that check for musl, use the
And even with these possible issues, they still use this method a lot and it's very reliable as no lib maintainer has opened an issue to complain that it's impossible to use So the question is, to introduce a new API in NodeJS, how accurate should it be? And even though I opened this thread to discuss exposing the Also, I opened a PR to improve the detection on I'm not an expert in this field so I don't know exactly how much accurate this solution will be but I think it could be more reliable than If we could do this same method directly C++ side, it could be even faster and we can fallback to Finally, I'll try to open a PR on NodeJS to discuss the possible implementation of |
To ask the question is to answer it, isn't it? :-) It can't be half-baked. |
If we agree that the current way of using If we want a more precise way we can look at What do you think could be the best strategy here? |
Neither, for the reasons outlined above. |
So, let's separate into two main problems: Expose libc versionThose who want to get the information about the libc and the libraries that want to check if musl is supported can also check this information. It will not cover all the edge cases of About the implementation, according to #41338 (review), maybe Expose function to check for muslMaybe we can export this function from
The most reliable way of checking should be
Both problems are solved in user space but with slow down on startup, and while we can speed up the main library ( |
I think you're trying to argue that approach X is less wrong than Y. Maybe, but it's irrelevant - they're still both wrong. It's like saying you're less pregnant now. "But performance" is not a credible argument either. It's not a performance-sensitive thing. Even if it was, well hurray, you give the wrong answer faster now. The premise is simply flawed. It's no use opening pull requests because they will only get rejected. |
Since we couldn't come to a consensus on what approach we should take for this, I'll close this issue as I'm satisfied with solving this problem in userland with the current strategy of detecting |
This topic already been discussed before in the following issues/PRs:
While this was considered a resolved topic as we can get the information from
process.report.getReport
, I want to put more focus on the performance implications of not exposing this information.Current Performance
Below, is the op/s to check if the system is
musl
using many methods I saw using this code search:benchmark.js
Above, is just the time spent in a benchmark, in real life, v8 will not optimize this function soo much, below the profile of swcx (rust binding for
swc
).Implications
These libraries usually implement their own version instead of installing a library like detect-libc to have a standardized way to check if the system is
musl
. This means that almost everynodejs
package that checks if it ismusl
will increase the startup time by some range of 5~30ms.But even usingdetect-libc
, it didn't cache the result value if the system ismusl
, so the problem with initialization is also a problem here, and this library is downloaded 9 million times, which means a lot of time can be saved here.Actually,
detect-libc
caches the value after the first run, so if multiple libraries use this lib to check formusl
, they can benefit from this cache.This issue can also affect
NPM
or any package manager if they decide to follow this RFC: npm/rfcs#519 if they decide to provide support for npm/rfcs#519 (comment). With this implementation, possibly every x64 linux will have some startup penalty but at least this will only be once, as the library no longer needs to do this check.Conclusion
There is a lot of time that can be saved by just exposing that information directly instead of relying on
process.report.getReport
. The main benefit will be for many libraries that expose native bindings with NodeJS.So, I want to raise some feedback about this, especially from @styfle, @Brooooooklyn, @richardlau and @bnoordhuis.
The text was updated successfully, but these errors were encountered: