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

lchown sync improvement #50007

Closed
wants to merge 1 commit into from
Closed

Conversation

IlyasShabi
Copy link
Contributor

Local benchmark

I used bench-chownSync.js from this PR

fs/bench-chownSync.js n=10000 method='lchownSync' type='existing'            ***     25.67 %       ±1.11% ±1.48% ±1.93%
fs/bench-chownSync.js n=10000 method='lchownSync' type='non-existing'        ***      4.90 %       ±1.09% ±1.46% ±1.92%

Ref: nodejs/performance#106

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Oct 1, 2023
@joyeecheung
Copy link
Member

#49913 has landed. Can you move the JS code back to lib/fs.js and switch to use SyncCallAndThrowOnError in the original implementation instead of introducing a new binding? Thanks.

@anonrig
Copy link
Member

anonrig commented Oct 3, 2023

This seems to be the duplicate of #49962

@IlyasShabi
Copy link
Contributor Author

Thanks @anonrig Im closing my PR

@IlyasShabi IlyasShabi closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants