GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: improve error perf of sync `lstat`+`fstat` by CanadaHonk · Pull Request #49868 · nodejs/node

Conversation

@CanadaHonk

@CanadaHonk CanadaHonk commented Sep 26, 2023

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tonivj5 and styfle reacted with heart emoji tonivj5 and styfle reacted with rocket emoji
All reactions
  • ❤️ 2 reactions
  • 🚀 2 reactions
@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 file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Sep 26, 2023
@CanadaHonk CanadaHonk mentioned this pull request Sep 26, 2023
Performance of node:fs nodejs/performance#106
Open
anonrig
anonrig approved these changes Sep 26, 2023
View reviewed changes
@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. Only starts once the PR has an approving review. labels Sep 26, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 26, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CanadaHonk

CanadaHonk commented Sep 26, 2023

Copy link
Copy Markdown
Member Author

Flaky CI?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@debadree25

debadree25 commented Sep 26, 2023

Copy link
Copy Markdown
Contributor

Failures look related
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Sep 27, 2023

Copy link
Copy Markdown
Member

@CanadaHonk It seems the tests are failing in this pr
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung reviewed Sep 27, 2023
View reviewed changes

@joyeecheung joyeecheung left a comment

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@joyeecheung

joyeecheung commented Oct 2, 2023

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the perf-lstat-fstat branch from 131b7ab to b32f918 Compare October 11, 2023 18:54
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 11, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 11, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the perf-lstat-fstat branch from b32f918 to 749a2ab Compare October 11, 2023 19:09
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 11, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 11, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung reviewed Oct 12, 2023
View reviewed changes
Comment thread benchmark/fs/bench-statSync-failure.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_file.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the perf-lstat-fstat branch from 749a2ab to 44ee0a1 Compare October 15, 2023 17:39
@anonrig
anonrig requested a review from joyeecheung October 15, 2023 17:39
@anonrig
anonrig force-pushed the perf-lstat-fstat branch 2 times, most recently from 113f98f to 402f2b7 Compare October 17, 2023 18:13
joyeecheung
joyeecheung approved these changes Oct 19, 2023
View reviewed changes
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 19, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 19, 2023
jasnell
jasnell approved these changes Oct 28, 2023
View reviewed changes
@debadree25 debadree25 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 28, 2023
@CanadaHonk
fs: improve error perf of sync lstat+fstat
2903256
@CanadaHonk
CanadaHonk force-pushed the perf-lstat-fstat branch from 402f2b7 to 2903256 Compare November 19, 2023 20:59
@CanadaHonk

CanadaHonk commented Nov 19, 2023

Copy link
Copy Markdown
Member Author

Rebased and fixed merge conflicts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 19, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 19, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 19, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CanadaHonk

CanadaHonk commented Nov 21, 2023

Copy link
Copy Markdown
Member Author

I think flaky CI?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 21, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Nov 22, 2023
View reviewed changes
@anonrig anonrig added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 23, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 23, 2023
@nodejs-github-bot
nodejs-github-bot merged commit ea88a3e into nodejs:main Nov 23, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 23, 2023

Copy link
Copy Markdown
Collaborator

Landed in ea88a3e
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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]>
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
@CanadaHonk Lei Shi
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]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
v21.3.0 proposal #50954
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]>
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]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
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]>
daniellockyer added a commit to daniellockyer/node that referenced this pull request Oct 10, 2026
@daniellockyer
fs: fix do_not_throw_error argument index in sync fstat
2ea782d 
The synchronous branch of the fstat binding read its
do_not_throw_error flag from args[2], which is always undefined on
that branch, so the flag was ignored and the binding always threw.

Read the flag from args[3] as documented, and restore tryStatSync()
to closing the file descriptor it owns before rethrowing the fstat
error, which was its behaviour before the flag was introduced.

Refs: nodejs#49868
Assisted-by: Cursor
Signed-off-by: Daniel Lockyer <[email protected]>
@daniellockyer daniellockyer mentioned this pull request Oct 10, 2026
fs: fix do_not_throw_error argument index in sync fstat #66637
Open
daniellockyer added a commit to daniellockyer/node that referenced this pull request Oct 10, 2026
@daniellockyer
fs: fix do_not_throw_error argument index in sync fstat
593b5bd 
The synchronous branch of the fstat binding read its
do_not_throw_error flag from args[2], which is always undefined on
that branch, so the flag was ignored and the binding always threw.

Read the flag from args[3] as documented, and restore tryStatSync()
to closing the file descriptor it owns before rethrowing the fstat
error, which was its behaviour before the flag was introduced.

Refs: nodejs#49868
Assisted-by: MiniMax M3.1 Flash Preview + Opus 5.5
Signed-off-by: Daniel Lockyer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@anonrig anonrig anonrig approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to file-system APIs and the fs module. 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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@CanadaHonk @nodejs-github-bot @debadree25 @anonrig @joyeecheung @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.