fs: improve error performance for `readSync` by pluris · Pull Request #50033 · nodejs/node

pluris · 2023-10-04T02:28:03Z

fs.readSync(fd, buffer, offset, length[, position])
fs.readSync(fd, buffer[, options])

 confidence improvement accuracy (*) (**) (***)
fs/bench-readSync.js n=10000 type='existing' 3.73 % ±6.45% ±8.58% ±11.17%
fs/bench-readSync.js n=10000 type='non-existing' *** 84.98 % ±10.30% ±13.80% ±18.14%
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 · 2023-10-04T10:37:02Z

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

nodejs-github-bot · 2023-10-04T12:41:53Z

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

joyeecheung · 2023-10-04T15:21:02Z

@@ -157,6 +157,7 @@ declare namespace InternalFSBinding {
 function read(fd: number, buffer: ArrayBufferView, offset: number, length: number, position: number, req: FSReqCallback<number>): void;
 function read(fd: number, buffer: ArrayBufferView, offset: number, length: number, position: number, req: undefined, ctx: FSSyncContext): number;


Suggested change
function read(fd: number, buffer: ArrayBufferView, offset: number, length: number, position: number, req: undefined, ctx: FSSyncContext): number;

joyeecheung · 2023-10-04T16:48:46Z

+ case 'offset-and-length':
+ for (let i = 0; i < n; i++) {
+ try {
+ fs.readSync(fd, Buffer.alloc(1), 0, 1, 0);


I think the buffer should be reused, as what's normally done in the wild. Also "reading a file from offset 1 many many times, one character at a time" do not seem to be a very typical use case. It would be better to tweak the benchmark so that it reads a big file in a reused buffer (with a length of fs.statSync(file).blksize) and then read without offset until the file ends, which is the most common way to use this API. We probably do not need an additional case for the offset as reading a file repeatedly from the same offset is probably not common enough in the wild to optimize for.

Thank you for your good comments.
I tried modifying it, but I'm not sure if it worked as you said.
Would it be better to remove paramType and test it by passing only buffer?

joyeecheung · 2023-10-04T16:49:45Z

+ try {
+ fs.readSync(fd, Buffer.alloc(1), 0, 1, 0);
+ } catch {
+ // Continue regardless of error.


Shouldn't this fail if type is existing?

It doesn't happen.
I tested only existing separately.

It doesn't happen.
I tested only existing separately.
Do you mean you only run this benchmark with existing? But this would still be part of the benchmark when e.g. the entire fs benchmark is run. So we should still avoid invalid/redundant combinations to prevent the benchmark from spending too much time on those and increasing the total runtime of the benchmark.
For now it seems the offset-and-length case is redundant, the no-offset-and-length case is already enough. The ultimate difference between 'offset-and-length' and 'no-offset-and-length' actually comes down to the position parameter - both actually would have the same offset and length after argument handling. But 'offset-and-length' would have the position set to 0 while the 'no-offset-and-length' would use the default position null (which means it will continue to read from the current position). I don't think this difference is really worth a separate benchmark because by testing them both we are only going to see how the underlying file system and the actual disk perform in difference cases after we remove what's in Node.js's control, but that isn't really something we need to care about. I think we can just keep the offset-and-length one and remove no-offset-and-length.

Benchmark is performed on non-existing and existing.
What I meant was that I separately confirmed that existing did not fail.
no-offset-and-length was removed.

pluris · 2023-10-04T23:59:10Z

Benchmark has been updated.

aduh95 · 2023-10-06T14:31:25Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1440

 confidence improvement accuracy (*) (**) (***)
fs/bench-readSync.js n=10000 type='existing' 1.60 % ±7.67% ±10.21% ±13.31%
fs/bench-readSync.js n=10000 type='non-existing' *** 77.28 % ±10.83% ±14.47% ±18.96%

nodejs-github-bot · 2023-10-10T20:36:01Z

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

nodejs-github-bot · 2023-10-10T22:18:00Z

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

nodejs-github-bot · 2023-10-11T02:19:42Z

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

anonrig · 2023-10-14T19:46:44Z

@pluris Can you rebase and force push? This issue is fixed in main branch.

aduh95 · 2023-10-14T21:41:17Z

@anonrig I don't think a rebase is necessary, you just need to restart a full CI, and the CI will rebase the PR commits on top of the base branch.

nodejs-github-bot · 2023-10-14T21:42:04Z

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

nodejs-github-bot · 2023-10-14T23:48:08Z

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

nodejs-github-bot · 2023-10-20T23:30:16Z

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

nodejs-github-bot · 2023-10-21T02:00:03Z

Landed in 506858b

PR-URL: #50033 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.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. typings labels Oct 4, 2023

pluris force-pushed the perf/readsync branch 2 times, most recently from d0eaf29 to 5f80eaeCompare October 4, 2023 02:31

mscdex added the needs-citgm PRs that need a CITGM CI run. label Oct 4, 2023

pluris changed the title ~~fs: improve performance for readSync~~fs: improve error performance for readSyncOct 4, 2023

pluris force-pushed the perf/readsync branch 2 times, most recently from 593d4ba to e757bbeCompare October 4, 2023 02:35

pluris changed the title ~~fs: improve error performance for readSync~~fs: improve error performance for readSyncOct 4, 2023

fs: improve error performance for readSync
4e9ea4b

pluris force-pushed the perf/readsync branch from e757bbe to 4e9ea4bCompare October 4, 2023 02:42

anonrig reviewed Oct 4, 2023
View reviewed changes

Comment threadsrc/node_file.cc

anonrig mentioned this pull request Oct 4, 2023
Performance of node:fsnodejs/performance#106
Open

anonrig reviewed Oct 4, 2023
View reviewed changes

Comment threadbenchmark/fs/bench-readSync.js Outdated

fixup! fs: improve error performance for readSync
da75cb5

pluris requested a review from anonrig October 4, 2023 04:40

mscdex removed the needs-citgm PRs that need a CITGM CI run. label Oct 4, 2023

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 pending requests for changes, and a CI started. labels Oct 4, 2023

anonrig approved these changes Oct 4, 2023
View reviewed changes

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

anonrig requested a review from joyeecheung October 4, 2023 14:34

joyeecheung reviewed Oct 4, 2023
View reviewed changes

fs: update benchmark and comments
ca4a904

pluris requested a review from joyeecheung October 4, 2023 23:59

joyeecheung reviewed Oct 9, 2023
View reviewed changes

Comment threadbenchmark/fs/bench-readSync.js Outdated

benchmark: move tmpfile creation to benchmark main
e75131a

pluris requested a review from joyeecheung October 10, 2023 00:12

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

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

anonrig approved these changes Oct 10, 2023
View reviewed changes

github-actionsBot mentioned this pull request Oct 11, 2023
CI Reliability 2023-10-11 nodejs/reliability#690
Open
30 tasks

joyeecheung approved these changes Oct 12, 2023
View reviewed changes

anonrig approved these changes Oct 21, 2023
View reviewed changes

anonrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 21, 2023

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

nodejs-github-bot merged commit 506858b into nodejs:mainOct 21, 2023

targos pushed a commit that referenced this pull request Oct 23, 2023
fs: improve error performance for readSync
0cc176e
PR-URL: #50033 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

targos mentioned this pull request Oct 23, 2023
v21.1.0 release proposal #50335
Merged

pluris deleted the perf/readsync branch November 8, 2023 02:24

targos pushed a commit that referenced this pull request Nov 11, 2023
fs: improve error performance for readSync
e49ebf8
PR-URL: #50033 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.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

		@@ -157,6 +157,7 @@ declare namespace InternalFSBinding {
		function read(fd: number, buffer: ArrayBufferView, offset: number, length: number, position: number, req: FSReqCallback<number>): void;
		function read(fd: number, buffer: ArrayBufferView, offset: number, length: number, position: number, req: undefined, ctx: FSSyncContext): number;

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 4, 2023

Uh oh!

nodejs-github-bot commented Oct 4, 2023

Uh oh!

joyeecheungOct 4, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plurisOct 4, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungOct 4, 2023

Choose a reason for hiding this comment

Uh oh!

plurisOct 4, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungOct 5, 2023

Choose a reason for hiding this comment

Uh oh!

plurisOct 5, 2023

Choose a reason for hiding this comment

Uh oh!

pluris commented Oct 4, 2023

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 10, 2023

Uh oh!

nodejs-github-bot commented Oct 10, 2023

Uh oh!

nodejs-github-bot commented Oct 11, 2023

Uh oh!

anonrig commented Oct 14, 2023

Uh oh!

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

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 14, 2023

Uh oh!

nodejs-github-bot commented Oct 14, 2023

Uh oh!

nodejs-github-bot commented Oct 20, 2023

Uh oh!

nodejs-github-bot commented Oct 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pluris commented Oct 4, 2023•
edited
Loading

joyeecheungOct 4, 2023•
edited
Loading

aduh95 commented Oct 6, 2023•
edited
Loading

aduh95 commented Oct 14, 2023•
edited
Loading