Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: improve error performance for fsyncSync #49880

Merged
merged 9 commits into from
Oct 20, 2023

Conversation

pluris
Copy link
Contributor

@pluris pluris commented Sep 26, 2023

                                                  confidence improvement accuracy (*)   (**)   (***)
fs/bench-fsyncSync.js n=10000 type='existing'            ***      3.37 %       ±1.49% ±1.99%  ±2.60%
fs/bench-fsyncSync.js n=10000 type='non-existing'        ***    116.95 %       ±6.64% ±8.87% ±11.64%

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 (***)

Refs: nodejs/performance#106

@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 the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 26, 2023
@pluris pluris changed the title fs: improve error preformance fori fsyncSync fs: improve error performance for fsyncSync Sep 26, 2023
benchmark/fs/bench-fsyncSync.js Outdated Show resolved Hide resolved
benchmark/fs/bench-fsyncSync.js Outdated Show resolved Hide resolved
benchmark/fs/bench-fsyncSync.js Show resolved Hide resolved
src/node_file.cc Outdated Show resolved Hide resolved
@@ -88,6 +88,11 @@ function close(fd) {
return binding.closeSync(fd);
}

function fsync(fd) {
fd = getValidatedFd(fd);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: If you moved this function to c++, it would improve the happy path (existing file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good opinion!
I think you just need to add CHECK_GE(fd,0) to C++.
Do you have a better idea?

Copy link
Member

Choose a reason for hiding this comment

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

We need to replicate the behavior of getValidatedFd in C++ and throw the correct error.

const getValidatedFd = hideStackFrames((fd, propName = 'fd') => {
  if (ObjectIs(fd, -0)) {
    return 0;
  }

  validateInt32(fd, propName, 0);

  return fd;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the implementation in C++.
I'm not sure if this implementation is correct.
also I updated the benchmark.
When I check it locally, the performance is strange.

Please review again.

src/node_file.cc Outdated Show resolved Hide resolved
@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 26, 2023
@pluris pluris force-pushed the perf/fsync branch 2 times, most recently from e1ebe02 to c710f94 Compare September 27, 2023 02:43
@pluris pluris requested a review from anonrig September 27, 2023 07:57
@pluris pluris force-pushed the perf/fsync branch 3 times, most recently from 764386f to ad76d06 Compare September 27, 2023 08:09
src/node_file.cc Outdated Show resolved Hide resolved
src/node_file.cc Outdated Show resolved Hide resolved
@anonrig anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 27, 2023
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

I think the diff can be a lot simpler if you just change the sync branch of the original implementations instead of repeating the code in a new binding..(and if you only introduce new bindings, the original sync branch would be dead code..) also I think this breaks --trace-sync-io?

@anonrig
Copy link
Member

anonrig commented Sep 30, 2023

Hey @pluris can you rebase this pull request? You'll see that sync.js file is now removed and merged into lib/fs.js

@anonrig anonrig requested a review from tniessen October 1, 2023 16:50
src/node_file.cc Outdated
@@ -116,6 +116,24 @@ inline int64_t GetOffset(Local<Value> value) {
return IsSafeJsInt(value) ? value.As<Integer>()->Value() : -1;
}

inline int GetValidatedFd(Environment* env, Local<Value> value) {
if (!value->IsInt32()) {
env->isolate()->ThrowException(ERR_INVALID_ARG_TYPE(
Copy link
Member

@joyeecheung joyeecheung Oct 2, 2023

Choose a reason for hiding this comment

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

I think this should be a ValidateInt32 if we want to start moving the validation to C++. Also this is not entirely on-par with the original implementation - the original implementation would print the value in a readable format in the case it's not a number, and this does not give any information about the invalid argument, which would make it harder for users to fix the error. We could also just don't move the error validation code now and leave it in JS if it's not ready to take on implementing proper argument validation + printing in C++.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joyeecheung Thank you for your comment.
You are right. As you said, I think we need to think more about the implementation of GetValidateFd(). First, I will remove this addition to C++ and change it to the original implementation.

@pluris pluris force-pushed the perf/fsync branch 2 times, most recently from cfd9354 to fcffde8 Compare October 2, 2023 14:01
typings/internalBinding/fs.d.ts Outdated Show resolved Hide resolved
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 20, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 20, 2023
@nodejs-github-bot nodejs-github-bot merged commit fbd08ec into nodejs:main Oct 20, 2023
27 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in fbd08ec

targos pushed a commit that referenced this pull request Oct 23, 2023
PR-URL: #49880
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49880
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@pluris pluris deleted the perf/fsync branch November 8, 2023 02:24
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49880
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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++. 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-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.

4 participants