Skip to content

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

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
Merged
nodejs-github-bot merged 1 commit into from
Nov 23, 2023
Merged

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

nodejs-github-bot merged 1 commit into from
Nov 23, 2023

Conversation

@CanadaHonk
Copy link
Member

@CanadaHonkCanadaHonk commented Sep 26, 2023

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-botnodejs-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
@CanadaHonkCanadaHonk mentioned this pull request Sep 26, 2023
Open
@anonriganonrig 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-actionsgithub-actionsbot 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

nodejs-github-bot commented Sep 26, 2023

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

@CanadaHonk
Copy link
MemberAuthor

CanadaHonk commented Sep 26, 2023

Flaky CI?

@debadree25
Copy link
Member

debadree25 commented Sep 26, 2023

Failures look related

@anonrig
Copy link
Member

anonrig commented Sep 27, 2023

@CanadaHonk It seems the tests are failing in this pr

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

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

joyeecheung commented Oct 2, 2023

#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.

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Oct 11, 2023

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

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Oct 11, 2023

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

joyeecheung
joyeecheung reviewed Oct 12, 2023
View reviewed changes
@anonriganonrigforce-pushed the perf-lstat-fstat branch 2 times, most recently from 113f98f to 402f2b7CompareOctober 17, 2023 18:13
@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Oct 19, 2023

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

This was referenced Oct 20, 2023
Open
Open
@debadree25debadree25 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
MemberAuthor

CanadaHonk commented Nov 19, 2023

Rebased and fixed merge conflicts.

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Nov 19, 2023

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

@CanadaHonk
Copy link
MemberAuthor

CanadaHonk commented Nov 21, 2023

I think flaky CI?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 21, 2023

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

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

nodejs-github-bot commented Nov 23, 2023

Landed in ea88a3e

targos pushed a commit that referenced this pull request Nov 23, 2023
@CanadaHonk@targos
fs: improve error perf of sync lstat+fstat
2a37ed3
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
@CanadaHonk@martenrichter
fs: improve error perf of sync lstat+fstat
5ba8608
PR-URL: nodejs#49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
@CanadaHonk
fs: improve error perf of sync lstat+fstat
154b8de
PR-URL: nodejs#49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
@RafaelGSSRafaelGSS mentioned this pull request Nov 28, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
@CanadaHonk@RafaelGSS
fs: improve error perf of sync lstat+fstat
9dc4cde
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 30, 2023
@CanadaHonk@RafaelGSS
fs: improve error perf of sync lstat+fstat
208bdd0
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@CanadaHonk@UlisesGascon
fs: improve error perf of sync lstat+fstat
e4f578c
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
@UlisesGasconUlisesGascon mentioned this pull request Dec 12, 2023
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 13, 2023
@CanadaHonk@UlisesGascon
fs: improve error perf of sync lstat+fstat
900d448
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 15, 2023
@CanadaHonk@UlisesGascon
fs: improve error perf of sync lstat+fstat
831eff4
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
@CanadaHonk@UlisesGascon
fs: improve error perf of sync lstat+fstat
c160adb
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Jan 9, 2024
@CanadaHonk@UlisesGascon
fs: improve error perf of sync lstat+fstat
05e25e0
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@anonriganonriganonrig approved these changes

@joyeecheungjoyeecheungjoyeecheung approved these changes

Assignees

No one assigned

Labels

author readyPRs 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++.fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.performanceIssues 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@debadree25@anonrig@joyeecheung@jasnell