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

Add runtime information about libc version #48204

Closed
H4ad opened this issue May 27, 2023 · 19 comments
Closed

Add runtime information about libc version #48204

H4ad opened this issue May 27, 2023 · 19 comments

Comments

@H4ad
Copy link
Member

H4ad commented May 27, 2023

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:

process.report.getReport() x 218 ops/sec ±4.16% (76 runs sampled)
isMusl x 167 ops/sec ±1.94% (74 runs sampled)
isMuslByFile x 95,904 ops/sec ±2.67% (80 runs sampled)
Fastest is isMuslByFile
benchmark.js
const Benchmark = require('benchmark');

const suite = new Benchmark.Suite();

const decoder = new TextDecoder();

const isMusl = () => {
  try {
    const lddPath = decoder.decode(require('child_process').execSync('ldd --version')).includes('musl')
    return lddPath
  } catch (e) {
    return true
  };
}

function isMuslByFile() {
  try {
    return readFileSync('/usr/bin/ldd', 'utf8').includes('musl')
  } catch (e) {
    return true
  }
}

suite.add('process.report.getReport()', function () {
  const id = !process.report.getReport().header.glibcVersionRuntime;
});

suite.add(`isMusl`, function () {
  const result = isMusl();
});

suite.add(`isMuslByFile`, function () {
  const result = isMuslByFile();
});

suite
  // add listeners
  .on('cycle', function (event) {
    console.log(String(event.target));
  })
  .on('complete', function () {
    console.log('Fastest is ' + this.filter('fastest').map('name'));
  })
  .run({
    async: true,
  });

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).

image

Command: node --cpu-prof swcx/index.js
Profile: swcx.cpuprofile.zip

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 every nodejs package that checks if it is musl will increase the startup time by some range of 5~30ms.

But even using detect-libc, it didn't cache the result value if the system is musl, 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 for musl, 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.

@mscdex
Copy link
Contributor

mscdex commented May 27, 2023

I want to put more focus on the performance implications of not exposing this information.

Why? Have you actually found this to be a major bottleneck somehow?

@merceyz
Copy link
Member

merceyz commented May 27, 2023

I want to put more focus on the performance implications of not exposing this information.

Why? Have you actually found this to be a major bottleneck somehow?

Yes, see #46060.

@mscdex
Copy link
Contributor

mscdex commented May 27, 2023

Yes, see #46060.

So then this issue is a duplicate of #46060?

@H4ad
Copy link
Member Author

H4ad commented May 27, 2023

Why? Have you actually found this to be a major bottleneck somehow?

I don't know what you mean by major bottleneck but spending more than 5ms just to check if the system is musl when the information can be static sounds to me a thing that we can optimize.

So then this issue is a duplicate of #46060?

No, my intention is not to try to make getReport faster, is just to expose the information of libc to be used by any rust library.

getReport is way more difficult to optimize and due to the nature of JS<->C++ serialization.

@H4ad
Copy link
Member Author

H4ad commented May 27, 2023

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.

@mscdex
Copy link
Contributor

mscdex commented May 27, 2023

I don't know what you mean by major bottleneck but spending more than 5ms just to check if the system is musl when the information can be static sounds to me a thing that we can optimize.

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 getReport() (once the previously mentioned DNS-related issue has a workaround), so even going from something like 500ms to 5ms is not going to be noticeable, especially for a task (installation of packages) that happens once.

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.

Again, improving getReport() should make this a non-issue. Additionally since napi-rs contains a fallback, perhaps it should just use that all the time if that is deemed to be faster?

@H4ad
Copy link
Member Author

H4ad commented May 27, 2023

so even going from something like 500ms to 5ms is not going to be noticeable, especially for a task (installation of packages) that happens once.

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 musl in that RFC, which I don't know if or when it will happen.

Additionally since napi-rs contains a fallback, perhaps it should just use that all the time if that is deemed to be faster?

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

@aqrln
Copy link
Contributor

aqrln commented Jun 19, 2023

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 getReport() (once the previously mentioned DNS-related issue has a workaround), so even going from something like 500ms to 5ms is not going to be noticeable, especially for a task (installation of packages) that happens once.

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 process.report.getReport() to get libc information. However, I wanted to provide some additional context to this discussion, as this seems relevant.

@bnoordhuis
Copy link
Member

This isn't perfectly solvable because there is no reliable way to feature-detect musl libc.

The approach that process.report.getReport() takes (check the system's dynamic linker) is... let's be diplomatic and call it "best effort" (read: flawed, can produce both false positives and false negatives.)

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.

@aqrln
Copy link
Contributor

aqrln commented Jun 20, 2023

This isn't perfectly solvable because there is no reliable way to feature-detect musl libc.

The approach that process.report.getReport() takes (check the system's dynamic linker) is... let's be diplomatic and call it "best effort" (read: flawed, can produce both false positives and false negatives.)

Good to know, I just assumed this was solvable at compile time, and without checking the code I also assumed that what getReport was doing was more reliable than the current userland approaches. Thanks so much for correcting these assumptions.

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.

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.

@bnoordhuis
Copy link
Member

If node is statically linked, then libc doesn't matter since dynamic loading is not supported anyway.

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.)

@aqrln
Copy link
Contributor

aqrln commented Jun 20, 2023

I see, thanks for clarification.

@H4ad
Copy link
Member Author

H4ad commented Jun 20, 2023

Currently, all the NodeJS (>10.x) applications that check for musl, use the getReport.

The approach that process.report.getReport() takes (check the system's dynamic linker) is... let's be diplomatic and call it "best effort" (read: flawed, can produce both false positives and false negatives.)

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 musl and NodeJS.

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 libc version, my main goal is to have a quick way to detect musl, so if we limit our implementation to just exposing that information, I think everyone will be satisfied.


Also, I opened a PR to improve the detection on detect-libc to detect via /usr/bin/ldd.

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 getReport since we check directly in the linker and much faster (30ms to 0.16ms).

If we could do this same method directly C++ side, it could be even faster and we can fallback to getReport when we don't have access to that file because of the permission model.


Finally, I'll try to open a PR on NodeJS to discuss the possible implementation of musl after my PR on detect-libc, so the discussion will be clearer as we will have some code to read/execute.

@bnoordhuis
Copy link
Member

So the question is, to introduce a new API in NodeJS, how accurate should it be?

To ask the question is to answer it, isn't it? :-) It can't be half-baked.

@H4ad
Copy link
Member Author

H4ad commented Jun 20, 2023

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 getReport is accurate enough, we just need to reopen #41338

If we want a more precise way we can look at /usr/bin/ldd like I'm doing in detect-libc, in this case, I can open a PR to do that.

What do you think could be the best strategy here?

@bnoordhuis
Copy link
Member

Neither, for the reasons outlined above.

@H4ad
Copy link
Member Author

H4ad commented Jun 20, 2023

So, let's separate into two main problems:

Expose libc version

Those 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 musl but at least will speed up detect-libc, which is downloaded 10M week.

About the implementation, according to #41338 (review), maybe process.versions is not the best place, but it can be inside process.

Expose function to check for musl

Maybe we can export this function from os, about the implementation, we have some strategies:

  • Read the file content of /usr/bin/ldd and check for the presence of musl, which will be the first option of detect-libc. (ref)
  • Use glibcVersionRuntime from getReport, the first option of detect-libc. (ref)
  • Use sharedObjects from getReport, the second option of detect-libc. (ref)
  • Run ldd --version and check for the presence of musl, the last option of detect-libc. (ref)

The most reliable way of checking should be sharedObjects, which accords to the musl faq:

Q: Where is ldd?
musl’s dynamic linker comes with ldd functionality built in. Just create a symlink from ld-musl-$ARCH.so to /bin/ldd. If the dynamic linker was started as “ldd”, it will detect that and print the appropriate DSO information.


Both problems are solved in user space but with slow down on startup, and while we can speed up the main library (detect-libc), many other libraries/applications have their own implementation (or they can't install detect-libc, eg: exports by napi-rs), so having a standardized solution in the NodeJS core might be more useful to the ecosystem than having multiple implementations.

@bnoordhuis
Copy link
Member

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.

@H4ad
Copy link
Member Author

H4ad commented Jul 19, 2023

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 musl using /usr/bin/ldd.

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

No branches or pull requests

6 participants
@mscdex @bnoordhuis @merceyz @aqrln @H4ad and others