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

fs: improve error perf of sync lstat+fstat #49868

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

CanadaHonk
Copy link
Member

Results from i7 Windows laptop:

                                                                                  confidence improvement accuracy (*)    (**)   (***)
fs\bench-statSync-failure.js throwType='noThrow' statSyncType='lstatSync' n=10000        ***     13.92 %       ±1.81%  ±2.42%  ±3.18%
fs\bench-statSync-failure.js throwType='noThrow' statSyncType='statSync' n=10000                  0.55 %       ±2.25%  ±3.00%  ±3.91%
fs\bench-statSync-failure.js throwType='throw' statSyncType='fstatSync' n=10000          ***    164.05 %       ±7.74% ±10.30% ±13.41%
fs\bench-statSync-failure.js throwType='throw' statSyncType='lstatSync' n=10000          ***     86.89 %       ±5.25%  ±7.02%  ±9.20%
fs\bench-statSync-failure.js throwType='throw' statSyncType='statSync' n=10000                   -1.38 %       ±2.51%  ±3.35%  ±4.38%
fs\bench-statSync.js statSyncType='fstatSync' n=10000                                             0.41 %       ±5.48%  ±7.30%  ±9.51%
fs\bench-statSync.js statSyncType='lstatSync' n=10000                                             0.25 %       ±3.59%  ±4.78%  ±6.22%
fs\bench-statSync.js statSyncType='statSync' n=10000                                             -1.60 %       ±3.55%  ±4.72%  ±6.14%

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 26, 2023
@anonrig anonrig added performance Issues and PRs related to the performance of Node.js. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2023
@nodejs-github-bot
Copy link
Collaborator

@CanadaHonk
Copy link
Member Author

Flaky CI?

@debadree25
Copy link
Member

Failures look related

@anonrig
Copy link
Member

anonrig commented Sep 27, 2023

@CanadaHonk It seems the tests are failing in this pr

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?

@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 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

@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

benchmark/fs/bench-statSync-failure.js Outdated Show resolved Hide resolved
src/node_file.cc Outdated Show resolved Hide resolved
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2023
@nodejs-github-bot
Copy link
Collaborator

@debadree25 debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 28, 2023
@CanadaHonk
Copy link
Member Author

Rebased and fixed merge conflicts.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2023
@nodejs-github-bot
Copy link
Collaborator

@CanadaHonk
Copy link
Member Author

I think flaky CI?

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 23, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 23, 2023
@nodejs-github-bot nodejs-github-bot merged commit ea88a3e into nodejs:main Nov 23, 2023
53 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ea88a3e

targos pushed a commit that referenced this pull request Nov 23, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
PR-URL: nodejs#49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
PR-URL: nodejs#49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 30, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
UlisesGascon pushed a commit that referenced this pull request Dec 13, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 15, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Jan 9, 2024
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants