fs: improve error performance of writevSync by IlyasShabi · Pull Request #50038 · nodejs/node

IlyasShabi · 2023-10-04T13:37:50Z

Local benchmark:

                                                  confidence improvement accuracy (*)   (**)  (***)
fs/bench-writevSync.js n=100000 type='invalid'        ***     125.24 %       ±2.51%   ±3.34%  ±4.37%
fs/bench-writevSync.js n=100000 type='valid'          ***     -0.26 %        ±3.07%   ±4.10%  ±5.34%

Ref: nodejs/performance#106

IlyasShabi · 2023-10-04T13:39:09Z

Feel free to suggest improvements in the benchmark file

Uzlopak · 2023-10-04T13:56:17Z

Afaik in c++ returning a value is slower than modifying a variable passed as parameter (by ref).

anonrig · 2023-10-04T14:30:29Z

benchmark/fs/bench-writevSync.js

+fs.writeFileSync(path, 'Some content.');
+
+const bench = common.createBenchmark(main, {
+  type: ['valid'],


Please add an invalid type to benchmarks

@anonrig I would appreciate help creating a good bench script for my first PR 🙏

lib/fs.js

anonrig · 2023-10-04T14:31:36Z

src/node_file.cc

-  } else {  // writeBuffers(fd, chunks, pos, undefined, ctx)
-    CHECK_EQ(argc, 5);
+  } else {
    FSReqWrapSync req_wrap_sync;


This line needs to be updated.

anonrig · 2023-10-04T14:32:21Z

benchmark/fs/bench-writevSync.js

+  bench.start();
+  for (let i = 0; i < n; i++) {
+    try {
+      fs.writevSync(fd, [buffer]);


In order to remove dead code elimination store the result of this operation and assert it when the benchmark finishes.

lib/fs.js

src/node_file.cc

mscdex · 2023-10-04T15:25:57Z

Did you swap the old and new node binary parameters when comparing the benchmarks or is that actually a 58% performance regression?

src/node_file.cc

mscdex · 2023-10-05T01:46:52Z

FWIW the changes currently result in:

fs/bench-writevSync.js n=100000 type='valid'                -0.83 %       ±7.81% ±10.40% ±13.55%

I'm guessing this should instead be more about improving the error case?

anonrig · 2023-10-05T15:50:03Z

benchmark/fs/bench-writevSync.js

+
+      bench.start();
+      for (let i = 0; i < n; i++) {
+        try {


We don't need a try/catch on valid part

anonrig · 2023-10-05T15:51:09Z

benchmark/fs/bench-writevSync.js

+      fs.closeSync(fd);
+      break;
+    case 'invalid':
+      fd = tmpdir.resolve(`.non-existing-file-${process.pid}`);


This is not a file descriptor. The variable name suggests fd

nodejs-github-bot · 2023-10-07T23:59:16Z

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

nodejs-github-bot · 2023-10-08T21:48:00Z

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

nodejs-github-bot · 2023-10-12T01:58:42Z

Landed in bf0f078

PR-URL: nodejs#50038 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: #50038 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: nodejs#50038 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

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 Oct 4, 2023

anonrig reviewed Oct 4, 2023

View reviewed changes

pluris reviewed Oct 4, 2023

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

pluris reviewed Oct 4, 2023

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

anonrig added the needs-benchmark-ci PR that need a benchmark CI run. label Oct 4, 2023

IlyasShabi force-pushed the writev-sync branch from c522003 to 1f60944 Compare October 4, 2023 19:47

mscdex reviewed Oct 5, 2023

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

IlyasShabi force-pushed the writev-sync branch from 1f60944 to 3c2e4ad Compare October 5, 2023 10:12

anonrig reviewed Oct 5, 2023

View reviewed changes

IlyasShabi force-pushed the writev-sync branch from 3c2e4ad to 2a2ca8b Compare October 7, 2023 17:05

fs: improve error performance of writevSync

4babf4d

IlyasShabi force-pushed the writev-sync branch from 2a2ca8b to 4babf4d Compare October 7, 2023 18:38

IlyasShabi changed the title ~~fs: improve writevSync performance~~ fs: improve error performance of writevSync Oct 7, 2023

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2023

anonrig approved these changes Oct 8, 2023

View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Oct 8, 2023

This was referenced Oct 9, 2023

CI Reliability 2023-10-09 nodejs/reliability#688

Open

CI Reliability 2023-10-10 nodejs/reliability#689

Open

CI Reliability 2023-10-11 nodejs/reliability#690

Open

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 12, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 12, 2023

nodejs-github-bot merged commit bf0f078 into nodejs:main Oct 12, 2023

RafaelGSS mentioned this pull request Oct 15, 2023

2023-10-17, Version 21.0.0 (Current) #49870

Merged

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023

fs: improve error performance of writevSync

41f3c3b

PR-URL: nodejs#50038 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

targos pushed a commit that referenced this pull request Nov 11, 2023

fs: improve error performance of writevSync

eb52f73

PR-URL: #50038 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

targos mentioned this pull request Nov 12, 2023

v20.10.0 release proposal #50682

Merged

juanarbol mentioned this pull request Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024

fs: improve error performance of writevSync

d75f4f9

PR-URL: nodejs#50038 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

Uh oh!

Conversation

IlyasShabi commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IlyasShabi commented Oct 4, 2023

Uh oh!

Uzlopak commented Oct 4, 2023

Uh oh!

anonrig Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

IlyasShabi Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anonrig Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

anonrig Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mscdex commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mscdex commented Oct 5, 2023

Uh oh!

anonrig Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

anonrig Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 7, 2023

Uh oh!

nodejs-github-bot commented Oct 8, 2023

Uh oh!

nodejs-github-bot commented Oct 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

IlyasShabi commented Oct 4, 2023 •

edited

Loading

mscdex commented Oct 4, 2023 •

edited

Loading