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

os: lazily load getOSInformation #50526

Closed
wants to merge 2 commits into from

Conversation

kylo5aby
Copy link
Contributor

@kylo5aby kylo5aby commented Nov 2, 2023

changes the strategy for obtaining operating system information
by implementing a lazy loading approach. Instead of immediately
invoking _getOSInformation() upon module load.

Fixes: #50525

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem. labels Nov 2, 2023
@kylo5aby kylo5aby changed the title lazily load getOSInformation os: lazily load getOSInformation Nov 2, 2023
@RaisinTen
Copy link
Contributor

I think the overhead you're referring to should disappear once the os module is added to the snapshot. Also, have you tried to measure how much improvement this has when you try to load the os module?

@kylo5aby
Copy link
Contributor Author

kylo5aby commented Nov 3, 2023

I think the overhead you're referring to should disappear once the os module is added to the snapshot. Also, have you tried to measure how much improvement this has when you try to load the os module?

actually I'm now trying to figure out some performance regression on os, seems there is a regression since 18.x, so I wonder if it's about the overhead from hungry mode, but as you mentioned, I have tried to benchmarked the performance about os.version, os.type, as well as os.cpus, os.loadavg etc, they are indeed performed as what you say, the differences between them are negligible

@kylo5aby kylo5aby force-pushed the lazy-get-osinformation branch from 39ed97c to 51aedc2 Compare November 3, 2023 04:17
@RaisinTen
Copy link
Contributor

seems there is a regression since 18.x

Is the regression specifically about the slowness of the require('os') call? Some benchmark showing how slow it became would be useful to take a look at. It might also be worth finding the commit that caused it to slow down.

@kylo5aby
Copy link
Contributor Author

kylo5aby commented Nov 3, 2023

seems there is a regression since 18.x

Is the regression specifically about the slowness of the require('os') call? Some benchmark showing how slow it became would be useful to take a look at. It might also be worth finding the commit that caused it to slow down.

Good point, I have run the benchmark of os, compare with 18.x and 22.x, the regression come from the cpus call:

node-benchmark-compare ./result_22pre_vs_18.csv
                                confidence improvement accuracy (*)   (**)  (***)
os/cpus.js n=30000                     ***    -27.79 %       ±0.49% ±0.66% ±0.87%
os/loadavg.js n=5000000                  *      0.76 %       ±0.64% ±0.85% ±1.11%
os/networkInterfaces.js n=10000          *      0.46 %       ±0.38% ±0.51% ±0.66%

and I use git blame and binary search to try to figure out the target version, and believe it is caused by libuv version

20.0.0 (uv version 1.44.2) vs 18 
os/cpus.js n=10000        ***      2.02 %       ±0.74% ±0.99% ±1.29%
20.2.0 (uv version 1.44.2) vs 18
os/cpus.js n=10000        ***      2.44 %       ±1.14% ±1.53% ±2.01%
20.3.0(uv version 1.45.0) vs 18 
os/cpus.js n=10000        ***    -29.91 %       ±1.18% ±1.59% ±2.11%
20.4.0 (uv version 1.46.0) vs 18
os/cpus.js n=10000        ***    -30.17 %       ±1.30% ±1.75% ±2.31%

@RaisinTen
Copy link
Contributor

What's the OS and arch of the system where this regression is reproducible?

@kylo5aby
Copy link
Contributor Author

kylo5aby commented Nov 3, 2023

What's the OS and arch of the system where this regression is reproducible?

it can be reproduced on an x64 Ubuntu 20.04.

@RaisinTen
Copy link
Contributor

Could you try to revert libuv/libuv#3861 and check if that fixes the regression?

@richardlau
Copy link
Member

It's almost certainly the libuv update. See libuv/libuv#4098.
For Node.js 18.x you should see a similar performance dip in 18.18.0 (or 18.18.1) which has the libuv update. The libuv update was reverted in Node.js 18.18.2 because of other functional regressions. A future Node.js 18 release may update to a future libuv release (e.g. proposed 1.47.0) which would then include libuv/libuv#3861 again.

@kylo5aby
Copy link
Contributor Author

kylo5aby commented Nov 3, 2023

I have compared the reverted version of libuv, it shows the similar performance differences to 18.x vs 22.x pre, the libuv PR you mentioned collects CPU information more accurately while introduces some system calls or allocation, which I believe is the main reason for the performance regression.

@RaisinTen
Copy link
Contributor

RaisinTen commented Nov 4, 2023

Looks like this is working as intended then. Should we go ahead and close this PR and #50525?

@kylo5aby
Copy link
Contributor Author

kylo5aby commented Nov 5, 2023

I‘ll close it, thanks for your advice

@kylo5aby kylo5aby closed this Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. os Issues and PRs related to the os subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

overhead on os when doesn't call os.version(), os.release(), os.type(), os.machine()
4 participants