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

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

/ node Public

fs: improve error performance of chownSync - #49748

Closed
anonrig wants to merge 3 commits into
nodejs:mainfrom
anonrig:fs-improve-chownsync
Closed

fs: improve error performance of chownSync#49748
anonrig wants to merge 3 commits into
nodejs:mainfrom
anonrig:fs-improve-chownsync

Conversation

anonrig commented Sep 21, 2023

Copy link
Copy Markdown
Member
                                                   confidence improvement accuracy (*)   (**)  (***)
fs/bench-chownSync.js n=10000 type='existing'                     0.24 %       ±2.31% ±3.09% ±4.06%
fs/bench-chownSync.js n=10000 type='non-existing'        ***     83.99 %       ±3.73% ±4.96% ±6.46%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 2 comparisons, you can thus expect the following amount of false-positive results:
  0.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.02 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

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 21, 2023
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. labels Sep 21, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023

Copy link
Copy Markdown
Collaborator

anonrig commented Sep 21, 2023

Copy link
Copy Markdown
Member Author

cc @nodejs/cpp-reviewers

Comment thread src/node_file.cc Outdated
Comment thread src/node_file.cc Outdated
anonrig force-pushed the fs-improve-chownsync branch from a67ad16 to 187fb83 Compare September 22, 2023 21:39

joyeecheung commented Sep 22, 2023
edited
Loading

Copy link
Copy Markdown
Member

Have you tried typing this function in the REPL and see if the system call happens before you type enter? (perhaps both our REPL and the DevTools console, we use different flags for them)

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023

Copy link
Copy Markdown
Collaborator

anonrig force-pushed the fs-improve-chownsync branch from 86a17b5 to d1ed9ff Compare September 26, 2023 15:28

anonrig commented Sep 26, 2023

Copy link
Copy Markdown
Member Author

Force pushed to resolve conflicts.

anonrig added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Sep 26, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2023

Copy link
Copy Markdown
Collaborator

joyeecheung 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

I just realized that this is not the right way to change the error throwing behavior. See #49593 (comment)

targos removed their request for review September 28, 2023 13:50

aduh95 commented Sep 29, 2023

Copy link
Copy Markdown
Contributor

This needs a rebase

aduh95 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 29, 2023

anonrig commented Sep 29, 2023

Copy link
Copy Markdown
Member Author

Closing in favor of #49962

anonrig closed this Sep 29, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. performance Issues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL