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 of `opendirSync` by anonrig · Pull Request #49705 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Sep 18, 2023

Copy link
Copy Markdown
Member 
fs/bench-opendirSync.js n=10000 bufferSize=128 type='existing'                    -0.15 %       ±1.56%  ±2.08%  ±2.71%
fs/bench-opendirSync.js n=10000 bufferSize=128 type='non-existing'        ***    109.56 %       ±3.93%  ±5.23%  ±6.81%
fs/bench-opendirSync.js n=10000 bufferSize=32 type='existing'                     -0.34 %       ±1.95%  ±2.60%  ±3.39%
fs/bench-opendirSync.js n=10000 bufferSize=32 type='non-existing'         ***    111.68 %       ±3.03%  ±4.03%  ±5.25%
fs/bench-opendirSync.js n=10000 bufferSize=4 type='existing'                      -1.19 %       ±1.44%  ±1.92%  ±2.51%
fs/bench-opendirSync.js n=10000 bufferSize=4 type='non-existing'          ***    104.85 %       ±8.18% ±11.01% ±14.59%

Ref: nodejs/performance#106

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, sonigeez, pluris, daniellockyer, rluvaton, MomenNano, kimathi-phil, gurgunday, and henriqueio reacted with rocket emoji
All reactions
  • 🚀 9 reactions
@anonrig
fs: improve error performance of opendirSync
4f8e64d
@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 18, 2023
@anonrig anonrig mentioned this pull request Sep 18, 2023
Performance of node:fs nodejs/performance#106
Open
@anonrig anonrig added performance Issues and PRs related to the performance of Node.js. needs-benchmark-ci PRs that need a benchmark CI run. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 18, 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 18, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 18, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54042/
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 Sep 18, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung reviewed Sep 18, 2023
View reviewed changes
Comment thread benchmark/fs/bench-opendirSync.js Resolved

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Sep 19, 2023
CI Reliability 2023-09-19 nodejs/reliability#670
Open
32 tasks
@anonrig
fixup! fs: improve error performance of opendirSync
134eee9
@anonrig
anonrig requested a review from joyeecheung September 19, 2023 00:28
@anonrig anonrig added review wanted PRs that need review. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 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 Sep 19, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 19, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

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

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

Could we improve error handling in JS in general before we move all of this into C++? I believe there's likely not a big difference anymore as soon as that's fixed.

Sorry, something went wrong.

Uh oh!

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

anonrig commented Sep 19, 2023

Copy link
Copy Markdown
Member Author

Could we improve error handling in JS in general before we move all of this into C++? I believe there's likely not a big difference anymore as soon as that's fixed.

@BridgeAR There is also the cost of crossing the C++ and JavaScript boundary that'll still be there even after we improve the error creation performance.
veyselsahin reacted with eyes 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 Sep 19, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54068/
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 Sep 20, 2023
CI Reliability 2023-09-20 nodejs/reliability#671
Open
27 tasks
@anonrig

anonrig commented Sep 20, 2023

Copy link
Copy Markdown
Member Author

cc @nodejs/cpp-reviewers can you review?
All reactions

Sorry, something went wrong.

Uh oh!

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

joyeecheung commented Sep 20, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Could we improve error handling in JS in general before we move all of this into C++?

For the simple FS errors I think it would probably be pretty hard to make the JS error creation performance on-par with the C++ one if we want to hide internal frames. If we throw from C++, we hide them for free, the stack is only going to be captured when the user access error.stack, if they don't need it there's no cost paid. If we have to throw from JS, we need to capture the stack trace eagerly to hide internal frames. And not having to capture the stack trace is always going to be faster than having to do that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung approved these changes Sep 20, 2023
View reviewed changes
@anonrig anonrig added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Sep 20, 2023
@github-actions github-actions Bot mentioned this pull request Sep 21, 2023
CI Reliability 2023-09-21 nodejs/reliability#672
Open
22 tasks
@targos

targos commented Sep 21, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Shouldn't openDir (the C++ one) be also updated to remove the sync code now that it's in another function?
All reactions

Sorry, something went wrong.

Uh oh!

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

anonrig commented Sep 21, 2023

Copy link
Copy Markdown
Member Author

Shouldn't openDir (the C++ one) be also updated to remove the sync code now that it's in another function?

@targos OpenDir is still used with callback implementation. After working on Sync functions, I'll move to callback implementations and make OpenDir like functions to only handle promise implementations.
All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in 571ecbf
All reactions

Sorry, something went wrong.

Uh oh!

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

RafaelGSS commented Sep 21, 2023

Copy link
Copy Markdown
Member

I see the needs-benchmark-ci label, but I don't see any benchmark ci. Just a possible local benchmarks in the PR description.
All reactions

Sorry, something went wrong.

Uh oh!

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

anonrig commented Sep 21, 2023

Copy link
Copy Markdown
Member Author

@RafaelGSS I didn't follow up with the label I've added. I don't think it's necessary for this pull request since it's highly unlikely to cause regression.
All reactions

Sorry, something went wrong.

Uh oh!

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

RafaelGSS commented Sep 21, 2023

Copy link
Copy Markdown
Member

But would be great to have actual reliable numbers to share. People might see this PR when scrolling the CHANGELOG.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@anonrig @ruyadorno
fs: improve error performance of opendirSync
b618fe2 
PR-URL: #49705
Refs: nodejs/performance#106
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
This was referenced Sep 28, 2023
v20.7.1 proposal #49917
Closed
v20.8.0 proposal #49932
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments
@benjamingr benjamingr benjamingr 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++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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. review wanted PRs that need review.

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

@anonrig @nodejs-github-bot @joyeecheung @targos @RafaelGSS @benjamingr @BridgeAR
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.