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

fix: cache family when calling libc() #121

Closed
wants to merge 1 commit into from
Closed

Conversation

styfle
Copy link

@styfle styfle commented Oct 28, 2024

We can assume that the same process will return the same report and thus we can cache the resulting family in memory to make it faster for subsequent invocations.

For example, this function is called in a loop here:
https://github.com/npm/cli/blob/780afc50e3a345feb1871a28e33fa48235bc3bd5/workspaces/arborist/lib/arborist/build-ideal-tree.js#L212

References

Fixes #116 (comment)

We can assume that the same process will return the same report and thus we can cache the resulting family in memory to make it faster for subsequent invocations.

https://github.com/npm/cli/blob/780afc50e3a345feb1871a28e33fa48235bc3bd5/workspaces/arborist/lib/arborist/build-ideal-tree.js#L212
@styfle styfle requested a review from a team as a code owner October 28, 2024 16:41
@Tofandel
Copy link
Contributor

Tofandel commented Oct 28, 2024

I just tried this approach in #120 but there also seems to be an issue with this when getReport is called later in the program it takes very very long to complete (think 3minutes which is forever in computer time, that might be a bug in node because cpu is at 0% during that time, or not, after all this method is meant for debugging only), caching it is still not enough, it needs to be ran before anything in the program runs because then it's instant (2ms on my machine)

Here is a log of console.time('report') and console.timeEnd('report') before and after the getReport call
when run in the end of the upgrade command:
⠼report: 3:10.573 (m:ss.mmm)
when run in the beginning of the process at the top level
report: 1.943ms

@Tofandel
Copy link
Contributor

Tofandel commented Oct 28, 2024

For anyone wondering why it's freezing and it's taking so much time to get a report, it has to do with this line which does a synchronus reverse DNS lookup in getReport https://github.com/nodejs/node/blob/5633c62219e199baac833e8862d60333d85dc3d3/src/node_report_utils.cc#L29

A simple test case

console.time("report");
process.report.getReport();
console.timeEnd("report");
await fetch('https://example.com/');
console.time("report");
process.report.getReport();
console.timeEnd("report");
await fetch('https://example.com/');
await fetch('https://example.com/');
console.time("report");
process.report.getReport();
console.timeEnd("report");

report: 1.909ms
report: 20.188s
report: 40.220s

This does look like a bug and I reported it to the node repo
nodejs/node#55576

@H4ad
Copy link

H4ad commented Nov 12, 2024

We can totally skip the getReport and use faster versions to detect the musl and libc, ref: nodejs/node#48831 (comment)

@Tofandel
Copy link
Contributor

Tofandel commented Nov 12, 2024

@H4ad Yep that's what yarn berry is doing as well, do you want to open a PR?

@H4ad
Copy link

H4ad commented Nov 12, 2024

@Tofandel I can, but only after my work shift (in 6hrs)

@wraithgar
Copy link
Member

If there are faster ways to do this that don't require process.spawn, that's good.

Otherwise we can look into caching this, but we'll have to do it in a way that doesn't break tests (which rely on this being able to change to test code paths). Typically this means isolating the detection to a separate file so that we can mock it in tests, bypassing the cache.

@wraithgar wraithgar closed this Nov 20, 2024
@wraithgar wraithgar reopened this Nov 20, 2024
@wraithgar
Copy link
Member

Sorry, hit close instead of comment.

@wraithgar
Copy link
Member

Closing in favor of #122, where the discussion appears to have happened the most

@wraithgar wraithgar closed this Nov 20, 2024
@styfle styfle deleted the patch-1 branch November 20, 2024 20:02
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.

4 participants