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

perf(detect-libc): faster musl check by looking for ldd file #19

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Jun 16, 2023

Closes #18

Now, we use by default the comparison of check if ldd file exists.

Performance

Before:

[family] Time Spent 30.49158699810505ms
[isNonGlibcLinux] Time Spent 29.517467997968197ms
[versionSync] Time Spent 26.344385005533695ms

npm run bench:calls

Now:

[family] Time Spent 0.15008499845862389ms
[isNonGlibcLinux] Time Spent 0.1661360003054142ms
[versionSync] Time Spent 0.184239000082016ms

npm run bench:calls

@H4ad
Copy link
Contributor Author

H4ad commented Jun 16, 2023

@lovell I don't know if perf is appropriated to describe this change since it could be a breaking change, so let me know if you want me to change to other message.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the PR, I've left a few thoughts and questions inline.

lib/detect-libc.js Outdated Show resolved Hide resolved
lib/detect-libc.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/unit.js Show resolved Hide resolved
benchmark/detect-libc.js Show resolved Hide resolved
lib/detect-libc.js Outdated Show resolved Hide resolved
@lovell
Copy link
Owner

lovell commented Jun 16, 2023

Could we use this same approach for a potential versionFromFilesystem()? For example, the /usr/bin/ldd file on some glibc-based Linux will contain the string literal "GLIBC X.Y".

@H4ad
Copy link
Contributor Author

H4ad commented Jun 16, 2023

In theory, we can:

image

This is the content of /usr/bin/ldd

We just need to build a faster parser to extract this information.

Do you want to include the change on version in this PR too?

@H4ad
Copy link
Contributor Author

H4ad commented Jun 17, 2023

@lovell How did you test linux musl? I'm trying to understand what is the output of cat /usr/bin/ldd on musl

@lovell
Copy link
Owner

lovell commented Jun 17, 2023

Although we can probably determine the family from the /usr/bin/ldd file on musl-based Linux, we are unlikely to be able to determine the version.

$ docker run -it --rm alpine:3.18 cat /usr/bin/ldd
#!/bin/sh
exec /lib/ld-musl-x86_64.so.1 --list "$@"

@H4ad H4ad requested a review from lovell June 17, 2023 14:45
@H4ad H4ad force-pushed the perf/faster-is-musl branch 2 times, most recently from 9f62abe to 1a66ea7 Compare June 17, 2023 14:50
@H4ad
Copy link
Contributor Author

H4ad commented Jul 5, 2023

Hey @lovell, any updates?

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you. Is there anything else you wanted to add, or is this good for release?

@H4ad
Copy link
Contributor Author

H4ad commented Jul 17, 2023

It's good for a release!

@lovell lovell merged commit e8c0afd into lovell:main Jul 18, 2023
35 checks passed
@lovell
Copy link
Owner

lovell commented Jul 18, 2023

v2.0.2 now available, tudo bom, obrigado!

@H4ad H4ad deleted the perf/faster-is-musl branch July 18, 2023 11:35
* This string is used to find if the {@link LDD_PATH} is musl
* @type {string}
*/
const MUSL_ON_LDD = MUSL.toLowerCase();
Copy link
Contributor

@styfle styfle Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This .toLowerCase() is redundant since MUSL on line 57 is already lowercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this just to make sure the text is always lowercase and a change to musl won't affect this constant.
But better than removing .toLowerCase could be just putting the string literal, since they are different information even though it is the same string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, might be best to inline those strings in the getFamilyFromLddContent() function since each is only used once, no need for a variable

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRs welcome 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #20

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.

Cache the results
3 participants