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.

Sign up for GitHub

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

Jump to bottom

fs: improve error perf of sync *times #49864

Closed
wants to merge 1 commit into from
Closed

fs: improve error perf of sync *times #49864

wants to merge 1 commit into from

Conversation

CanadaHonk
Copy link
Member

@CanadaHonk CanadaHonk commented Sep 25, 2023 •
edited
Loading

Results from i7 Windows laptop:

                                                                confidence improvement accuracy (*)    (**)   (***)
fs\bench-timesSync.js n=1000 func='futimes' type='existing'                     1.30 %       ±8.42% ±11.20% ±14.57%
fs\bench-timesSync.js n=1000 func='futimes' type='non-existing'        ***    105.93 %      ±12.64% ±16.88% ±22.09%
fs\bench-timesSync.js n=1000 func='lutimes' type='existing'                     5.56 %       ±8.48% ±11.29% ±14.73%
fs\bench-timesSync.js n=1000 func='lutimes' type='non-existing'        ***     62.51 %      ±13.07% ±17.43% ±22.75%
fs\bench-timesSync.js n=1000 func='utimes' type='existing'                     -1.04 %       ±5.85%  ±7.78% ±10.13%
fs\bench-timesSync.js n=1000 func='utimes' type='non-existing'         ***     61.88 %       ±8.31% ±11.06% ±14.40%

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 Sep 25, 2023
@CanadaHonk CanadaHonk mentioned this pull request Sep 25, 2023
Open
@anonrig anonrig added performance Issues and PRs related to the performance of Node.js. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54239/

@CanadaHonk
Copy link
Member Author

1 more review please? Thanks! @nodejs/fs @nodejs/cpp-reviewers

@anonrig
Copy link
Member

anonrig commented Sep 27, 2023

@CanadaHonk can you resolve the conflicts?

@anonrig anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 27, 2023
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54294/

joyeecheung
joyeecheung reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

I think the diff can be a lot simpler if you just change the sync branch of the original implementations instead of repeating the code in a new binding..(and if you only introduce new bindings, the original sync branch would be dead code..) also I think this breaks --trace-sync-io?

@anonrig
Copy link
Member

anonrig commented Sep 27, 2023

I think the diff can be a lot simpler if you just change the sync branch of the original implementations instead of repeating the code in a new binding..(and if you only introduce new bindings, the original sync branch would be dead code..) also I think this breaks --trace-sync-io?

I think differentiating promise and sync implementations are better to maintain and benchmark. I think this would make fast api implementations a lot harder.

@joyeecheung
Copy link
Member

@anonrig

I think differentiating promise and sync implementations are better to maintain and benchmark.

The async one isn't related to promises. I've laid out my reasons about maintainability in #49593 (comment)

I think this would make fast api implementations a lot harder.

Can you elaborate on why? I don't think there is a lot of differences between the sync and the async version - note that we do not call the async callback immediately, this means we can include them in the fast API just fine. Essentially the sync v.s. async part really just comes down to what you pass to the libuv methods.

This was referenced Sep 28, 2023
Open
Open
@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 anonrig removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 11, 2023
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54713/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54725/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54746/

anonrig pushed a commit that referenced this pull request Oct 12, 2023
@CanadaHonk @anonrig
fs: improve error perf of sync *times
6bd77db 
PR-URL: #49864
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@anonrig
Copy link
Member

anonrig commented Oct 12, 2023

Landed in 6bd77db

@anonrig anonrig closed this Oct 12, 2023
@RafaelGSS RafaelGSS mentioned this pull request Oct 15, 2023
Merged
@Trott
Copy link
Member

Trott commented Oct 15, 2023

https://ci.nodejs.org/job/node-test-pull-request/54746/

This was landed despite a failing Jenkins CI.

@anonrig
Copy link
Member

anonrig commented Oct 15, 2023

https://ci.nodejs.org/job/node-test-pull-request/54746/

This was landed despite a failing Jenkins CI.

Yes, because the failing test was fixed in main branch. It was caused by test runner concurrency cli flag.

@tniessen tniessen mentioned this pull request Oct 15, 2023
Closed
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@CanadaHonk @alexfernandez
fs: improve error perf of sync *times
23172d7 
PR-URL: nodejs#49864
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
@CanadaHonk @targos
fs: improve error perf of sync *times
8413670 
PR-URL: #49864
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos mentioned this pull request Nov 12, 2023
Merged
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@anonrig anonrig anonrig approved these changes

@Qard Qard Qard approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@CanadaHonk @nodejs-github-bot @anonrig @joyeecheung @Trott @Qard