-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: improve performance of hostname and homedir #50037
Conversation
f26183c
to
344e221
Compare
Updated the benchmark in initial post. We should discuss the caching. But this is the minimal consensus, which should be mergable. |
@Uzlopak do we have an idea how much time it takes in ms to get the homedir? If it's quick, I think we can land without a cache, the improvements are good enough. If it's slow, I think adding caching will not be bad, accords to docs: https://docs.libuv.org/en/v1.x/misc.html#c.uv_os_homedir doesn't look like this information will change during the execution of the process. We only need to worry about the snapshot. |
I dont know how to measure the lookup time |
Code: const {homedir, hostname} = require('os')
let now = performance.now();
let value = homedir();
console.log(`Diff: ${ performance.now() - now }ms`);
now = performance.now();
value = hostname();
console.log(`Diff: ${ performance.now() - now }ms`); Output:
Since we don't have any hot path calling this code multiple times, I think we can skip caching to not add complexity without a good reason. |
ah, you mean like that. Maybe programms like npm or pnpm need homedir more excessively. |
Package managers are more likely to have I/O or compression/decompression be their bottleneck rather than something like |
@anonrig can you please add the author-ready tag? |
@mcollina can i interest you into reviewing this PR? |
@anonrig |
The CI is locked. It seems the errors are fixed in main. Can you rebase and force push? |
216f85a
to
ed76554
Compare
@anonrig |
@anonrig |
@anonrig can you give the CI some love please? :D |
@anonrig |
Landed in 0f0dd1a |
PR-URL: nodejs#50037 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This comment was marked as off-topic.
This comment was marked as off-topic.
PR-URL: nodejs#50037 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: #50037 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
This PR improves the performance of the homedir and hostname.