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 performance for `rmdirSync` by CanadaHonk · Pull Request #49846 · nodejs/node

Conversation

@CanadaHonk

@CanadaHonk CanadaHonk commented Sep 25, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Results from i9/Linux/SSD:

                                                   confidence improvement accuracy (*)   (**)  (***)
fs/bench-rmdirSync.js n=10000 type='existing'                     0.54 %       ±1.35% ±1.80% ±2.34%
fs/bench-rmdirSync.js n=10000 type='non-existing'        ***     61.81 %       ±2.15% ±2.88% ±3.77%

Ref: nodejs/performance#106

Thanks to and inspiration from @anonrig 🚀

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, H4ad, and metcoder95 reacted with rocket emoji
All reactions
  • 🚀 3 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 25, 2023
anonrig
anonrig reviewed Sep 25, 2023
View reviewed changes
Comment thread lib/fs.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig mentioned this pull request Sep 25, 2023
Performance of node:fs nodejs/performance#106
Open
anonrig
anonrig reviewed Sep 25, 2023
View reviewed changes
Comment thread benchmark/fs/bench-rmdirSync.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread benchmark/fs/bench-rmdirSync.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread benchmark/fs/bench-rmdirSync.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@CanadaHonk
CanadaHonk force-pushed the perf-rmdirsync branch from f822ca4 to 9b7260d Compare September 25, 2023 03:26
@CanadaHonk

CanadaHonk commented Sep 25, 2023

Copy link
Copy Markdown
Member Author

Updated to fix all things mentioned in review, thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CanadaHonk CanadaHonk changed the title fs: improve performance for fs.rmdirSync fs: improve error performance for fs.rmdirSync Sep 25, 2023
anonrig
anonrig approved these changes Sep 25, 2023
View reviewed changes
@anonrig anonrig added performance Issues and PRs related to the performance of Node.js. author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. needs-benchmark-ci PRs that need a benchmark CI run. labels Sep 25, 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 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@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 Sep 25, 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 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@CanadaHonk CanadaHonk changed the title fs: improve error performance for fs.rmdirSync fs: improve error performance for rmdirSync Sep 25, 2023
@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 Sep 25, 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 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@CanadaHonk

CanadaHonk commented Sep 25, 2023

Copy link
Copy Markdown
Member Author

Flaky CI I think?
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

This comment was marked as outdated.

Sign in to view
@CanadaHonk

CanadaHonk commented Sep 26, 2023

Copy link
Copy Markdown
Member Author

Flaky CI again? 😅
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

This comment was marked as outdated.

Sign in to view
@richardlau richardlau 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 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
@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 15, 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 15, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 15, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54829/
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 Oct 17, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Oct 19, 2023
CI Reliability 2023-10-19 nodejs/reliability#691
Open
25 tasks
@CanadaHonk
fs: improve error performance for rmdirSync
f68494d
@CanadaHonk
CanadaHonk force-pushed the perf-rmdirsync branch from 78557c1 to f68494d Compare November 19, 2023 20:51
@CanadaHonk

CanadaHonk commented Nov 19, 2023

Copy link
Copy Markdown
Member Author

Rebased on latest main and updated description with rebenchmark.
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/55773/
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

Flaky CI?
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

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/55837/
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 23, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot commented Nov 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in f11b206
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
@CanadaHonk Lei Shi
fs: improve error performance for rmdirSync
31e837c 
PR-URL: nodejs#49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 27, 2023
@CanadaHonk @RafaelGSS
fs: improve error performance for rmdirSync
c4f9654 
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[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 performance for rmdirSync
483200f 
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@CanadaHonk @UlisesGascon
fs: improve error performance for rmdirSync
7a09209 
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[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 performance for rmdirSync
cb55d04 
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 15, 2023
@CanadaHonk @UlisesGascon
fs: improve error performance for rmdirSync
0d59dda 
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
@CanadaHonk @UlisesGascon
fs: improve error performance for rmdirSync
f94a24c 
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes
@H4ad H4ad H4ad 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-benchmark-ci PRs that need a benchmark CI run. 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.

7 participants

@CanadaHonk @nodejs-github-bot @anonrig @IlyasShabi @joyeecheung @H4ad @richardlau
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.