FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: improve error performance of `opendirSync` by anonrig · Pull Request #49705 · nodejs/node · GitHub

/ node Public

fs: improve error performance of opendirSync - #49705

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
anonrig:improve-fs-opendirSync
Sep 21, 2023
ConversationCommits (2)ChecksFiles changed
Merged

fs: improve error performance of opendirSync#49705
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
anonrig:improve-fs-opendirSync

Conversation

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

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 18, 2023
anonrig mentioned this pull request Sep 18, 2023
Open
anonrig added performance Issues and PRs related to the performance of Node.js. needs-benchmark-ci PR that need a benchmark CI run. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 18, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023

nodejs-github-bot commented Sep 18, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Sep 18, 2023

Copy link
Copy Markdown
Collaborator

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

joyeecheung reviewed Sep 18, 2023
View reviewed changes
Comment thread benchmark/fs/bench-opendirSync.js Outdated
Comment thread benchmark/fs/bench-opendirSync.js Outdated
github-actions Bot mentioned this pull request Sep 19, 2023
Open
32 tasks
anonrig requested a review from joyeecheung September 19, 2023 00:28
anonrig added review wanted PRs that need reviews. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 19, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 19, 2023

nodejs-github-bot commented Sep 19, 2023

Copy link
Copy Markdown
Collaborator

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

BridgeAR reviewed Sep 19, 2023
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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.

nodejs-github-bot commented Sep 19, 2023

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Sep 20, 2023
Open
27 tasks

anonrig commented Sep 20, 2023

Copy link
Copy Markdown
Member Author

cc @nodejs/cpp-reviewers can you review?

joyeecheung commented Sep 20, 2023 •
edited
Loading

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.

anonrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 20, 2023
github-actions Bot mentioned this pull request Sep 21, 2023
Open
22 tasks

targos commented Sep 21, 2023 •
edited
Loading

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?

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.

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 21, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 21, 2023
nodejs-github-bot merged commit 571ecbf into nodejs:main Sep 21, 2023

nodejs-github-bot commented Sep 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in 571ecbf

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.

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.

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.

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
fs: improve error performance of opendirSync
b618fe2 
PR-URL: #49705
Refs: nodejs/performance#106
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This was referenced Sep 28, 2023
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR left review comments
benjamingr benjamingr approved these changes
joyeecheung joyeecheung approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fs Issues and PRs related to the fs subsystem / file system. needs-benchmark-ci PR 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 reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL