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

Cache the results #18

Closed
H4ad opened this issue May 27, 2023 · 4 comments · Fixed by #19
Closed

Cache the results #18

H4ad opened this issue May 27, 2023 · 4 comments · Fixed by #19
Labels

Comments

@H4ad
Copy link
Contributor

H4ad commented May 27, 2023

Most of the functions depend on functions that are expensive to call, I even opened an issue on NodeJS to talk more about it nodejs/node#48204.

So, if more than one library depends on this library and the results from it, they will call the expensive functions twice, do you see some issue with caching it?

Also, some libraries uses a faster way of checking if musl is supported, like https://github.com/swc-project/cli/blob/master/src/swcx/index.ts#L78, I don't know why they use getReport but did you see any issue with this method?

@lovell
Copy link
Owner

lovell commented May 27, 2023

Hi, the response of getReport(), if available, is already cached as report.

let report = null;
const getReport = () => {
if (!report) {
/* istanbul ignore next */
report = isLinux() && process.report
? process.report.getReport()
: {};
}
return report;

The response of running the getconf ... ldd ... child process, if required, is already cached as commandOut.

const command = 'getconf GNU_LIBC_VERSION 2>&1 || true; ldd --version 2>&1 || true';
let commandOut = '';
const safeCommand = () => {
if (!commandOut) {
return new Promise((resolve) => {
childProcess.exec(command, (err, out) => {
commandOut = err ? ' ' : out;
resolve(commandOut);
});
});
}
return commandOut;

The benchmark timings for the underlying getReport() in the linked issue are surprising though. Perhaps we should remove its use entirely?

@H4ad
Copy link
Contributor Author

H4ad commented May 27, 2023

Oh nice, sorry for misreading the code, I will update my statement on the linked issue.

About removing the usage, I see some JS libraries that use readFileSync to check for musl, I assume can be used but I don't know is 100% safe as getReport.

The problem with readFileSync is: is just an optimistic evaluation, that file could not exist in the environment (I read it somewhere but I didn't find the source anymore).

But I also find cases in other languages where ldd is used to check for musl.

I think this could be released as a major version as we don't exactly know the potential issues, or am I being a lot safer?

@lovell
Copy link
Owner

lovell commented May 30, 2023

I tested this with a few flavours of Linux and versions of Node.js and can confirm the use of fs.readFile is currently always faster than getReport(), which itself is always faster than exec-ing a child process. We're talking ~2ms vs ~4ms vs ~6ms.

However the libc version number is not always available via the readFile method so we'd need all existing methods to remain in place.

@H4ad
Copy link
Contributor Author

H4ad commented May 30, 2023

So, we can optimize familly, which will speed up isNonGlibcLinuxSync and isNonGlibcLinux but there's nothing we can do to speed up when the user wants the version.
At least, those who want to check if the system is musl can get a little bit better performance.

If that's the case, I can open a PR to improve these methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants