Skip to content

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

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-botnodejs-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
@anonriganonrig 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-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonrig

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/cpp-reviewers

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
@joyeecheung

joyeecheung commented Sep 22, 2023

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)

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonrig

Copy link
Copy Markdown
MemberAuthor

Force pushed to resolve conflicts.

@anonriganonrig added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 26, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

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

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

Copy link
Copy Markdown
Contributor

This needs a rebase

@aduh95aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 29, 2023
@anonrig

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #49962

@anonriganonrig closed this Sep 29, 2023
Sign up for freeto 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++.fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@anonrig@nodejs-github-bot@joyeecheung@aduh95@targos@tniessen@RaisinTen