fs: improve `readFileSync` with file descriptors #49691

anonrig · 2023-09-17T20:47:19Z

This particular change applies to: fs.readFileSync(fs.openSync(__filename), 'utf8')

My local benchmarks are as follows:

fs/readFileSync.js n=10000 hasFileDescriptor='false' path='existing' encoding='undefined'                     0.73 %       ±1.65% ±2.20% ±2.86%
fs/readFileSync.js n=10000 hasFileDescriptor='false' path='existing' encoding='utf8'                          0.05 %       ±1.74% ±2.32% ±3.02%
fs/readFileSync.js n=10000 hasFileDescriptor='false' path='non-existing' encoding='undefined'        ***     62.27 %       ±2.82% ±3.77% ±4.95%
fs/readFileSync.js n=10000 hasFileDescriptor='false' path='non-existing' encoding='utf8'             ***     60.93 %       ±2.72% ±3.64% ±4.78%
fs/readFileSync.js n=10000 hasFileDescriptor='true' path='existing' encoding='undefined'                      0.22 %       ±1.47% ±1.96% ±2.56%
fs/readFileSync.js n=10000 hasFileDescriptor='true' path='existing' encoding='utf8'                  ***     88.18 %       ±2.56% ±3.43% ±4.49%
fs/readFileSync.js n=10000 hasFileDescriptor='true' path='non-existing' encoding='undefined'                 -3.31 %       ±4.32% ±5.80% ±7.65%
fs/readFileSync.js n=10000 hasFileDescriptor='true' path='non-existing' encoding='utf8'                      -3.18 %       ±3.58% ±4.79% ±6.30%

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

rluvaton · 2023-09-18T06:34:59Z

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

bnoordhuis · 2023-09-18T10:16:45Z

Test failures look related.

Correctness issue: file descriptors are int32, not uint32 (also on Windows, because they go through libuv.)

anonrig · 2023-09-18T14:34:02Z

@bnoordhuis We validate file descriptors on lib/fs.js by checking if it is Uint32 (ref: https://github.com/nodejs/node/blob/main/lib/fs.js#L204).

bnoordhuis · 2023-09-18T19:35:42Z

Apparently that goes back to commit 0803962 from 2015... still wrong though. :-)

(Mostly harmless although technically signed integer overflow is UB in C++.)

anonrig · 2023-09-18T19:41:21Z

still wrong though. :-)

Nice find! I'll update the pull request.

@RafaelGSS I'm getting some weird failed test cases where stdout and stderr are both empty in the failing cases, but returning a status of 1. Any idea?

AssertionError [ERR_ASSERTION]
    at Object.<anonymous> (/Users/runner/work/node/node/test/parallel/test-permission-fs-read.js:42:10)
    at Module._compile (node:internal/modules/cjs/loader:1264:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1318:10)
    at Module.load (node:internal/modules/cjs/loader:1113:32)
    at Module._load (node:internal/modules/cjs/loader:954:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:80:12)
    at node:internal/main/run_main_module:23:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: null,
  expected: 0,
  operator: 'strictEqual'
}

src/node_file.cc

anonrig · 2023-09-24T20:51:21Z

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

09:17:21                                                                                               confidence improvement accuracy (*)    (**)   (***)
09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='false' path='existing' encoding='undefined'                    -2.80 %       ±8.78% ±11.68% ±15.21%
09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='false' path='existing' encoding='utf8'                         -6.68 %       ±9.49% ±12.63% ±16.44%
09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='false' path='non-existing' encoding='undefined'                -3.47 %       ±9.74% ±12.97% ±16.88%
09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='false' path='non-existing' encoding='utf8'                     -5.20 %       ±8.47% ±11.26% ±14.66%
09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='true' path='existing' encoding='undefined'                     -1.99 %       ±4.82%  ±6.41%  ±8.34%
09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='true' path='existing' encoding='utf8'                  ***    126.70 %      ±16.25% ±21.75% ±28.56%
09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='true' path='non-existing' encoding='undefined'                 -1.31 %       ±6.23%  ±8.29% ±10.79%
09:17:21 fs/readFileSync.js n=10000 hasFileDescriptor='true' path='non-existing' encoding='utf8'              ***    143.95 %      ±18.72% ±25.19% ±33.36%

nodejs-github-bot · 2023-09-24T20:54:23Z

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

anonrig · 2023-09-24T20:56:57Z

@nodejs/cpp-reviewers @nodejs/fs Can you review?

bnoordhuis · 2023-09-25T07:02:27Z

src/node_file.cc

+      CHECK_EQ(0, uv_fs_close(nullptr, &req, file, nullptr));
+      FS_SYNC_TRACE_END(close);
+    }
+    uv_fs_req_cleanup(&req);


For correctness reasons this ought to be done after the uv_fs_read() call on line 2590. It works okay now because libuv doesn't allocate for a single buffer (i.e. this cleanup call is a no-op) but that's a lucky accident.

Does that mean that on every while loop iteration (for uv_fs_read), we need to call uv_fs_req_cleanup?

resolved

nodejs-github-bot · 2023-09-25T21:56:05Z

Landed in f16f41c

targos · 2023-09-26T06:44:26Z

@anonrig Please always post the benchmark CI results in the PR (for example by editing the comment with the link like I just did). Jenkins results are not permanent.

PR-URL: #49691 Reviewed-By: Stephen Belanger <[email protected]>

PR-URL: nodejs#49691 Reviewed-By: Stephen Belanger <[email protected]>

Fix a file permissions regression when `fs.readFileSync()` is called in append mode on a file that does not already exist introduced by the fast path for utf8 encoding. PR-URL: #52101 Fixes: #52079 Refs: #49691 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

Fix a file permissions regression when `fs.readFileSync()` is called in append mode on a file that does not already exist introduced by the fast path for utf8 encoding. PR-URL: nodejs#52101 Fixes: nodejs#52079 Refs: nodejs#49691 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

Fix a file permissions regression when `fs.readFileSync()` is called in append mode on a file that does not already exist introduced by the fast path for utf8 encoding. PR-URL: #52101 Fixes: #52079 Refs: #49691 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>

anonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 17, 2023

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 17, 2023

anonrig mentioned this pull request Sep 17, 2023

Performance of node:fs nodejs/performance#106

Open

rluvaton added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 17, 2023

anonrig force-pushed the fs-read-file-sync-fd branch 2 times, most recently from 0dfcfa4 to de56591 Compare September 18, 2023 19:53

anonrig requested a review from RafaelGSS September 20, 2023 15:06

RafaelGSS previously requested changes Sep 21, 2023

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

anonrig requested a review from RafaelGSS September 21, 2023 19:29

anonrig mentioned this pull request Sep 21, 2023

fs: fix file descriptor validator #49752

Merged

anonrig added 2 commits September 24, 2023 16:49

fs: improve readFileSync with file descriptors

28cf7ff

fixup! fs: improve readFileSync with file descriptors

a415654

anonrig force-pushed the fs-read-file-sync-fd branch from 6fea27f to a415654 Compare September 24, 2023 20:49

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

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

bnoordhuis reviewed Sep 25, 2023

View reviewed changes

Qard approved these changes Sep 25, 2023

View reviewed changes

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

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

nodejs-github-bot merged commit f16f41c into nodejs:main Sep 25, 2023
55 checks passed

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023

fs: improve readFileSync with file descriptors

07d0518

PR-URL: #49691 Reviewed-By: Stephen Belanger <[email protected]>

ruyadorno mentioned this pull request Sep 28, 2023

v20.7.1 proposal #49917

Closed

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023

fs: improve readFileSync with file descriptors

31702c9

PR-URL: #49691 Reviewed-By: Stephen Belanger <[email protected]>

ruyadorno mentioned this pull request Sep 28, 2023

v20.8.0 proposal #49932

Merged

This was referenced Oct 2, 2023

binding: mock readFileSync tschaub/mock-fs#378

Closed

Fix tests on Node 20.8.0+ getgauge/gauge-js#603

Merged

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

fs: improve readFileSync with file descriptors

1542939

PR-URL: nodejs#49691 Reviewed-By: Stephen Belanger <[email protected]>

richardlau added the dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. label Mar 15, 2024

This was referenced Mar 15, 2024

fs: fix edge case in readFileSync utf8 fast path #52101

Merged

fs.writeFileSync in Node 20 failed to write file in TMPDIR #52079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: improve `readFileSync` with file descriptors #49691

fs: improve `readFileSync` with file descriptors #49691

anonrig commented Sep 17, 2023 •

edited

Loading

rluvaton commented Sep 18, 2023

bnoordhuis commented Sep 18, 2023

anonrig commented Sep 18, 2023

bnoordhuis commented Sep 18, 2023

anonrig commented Sep 18, 2023

anonrig commented Sep 24, 2023 •

edited by targos

Loading

nodejs-github-bot commented Sep 24, 2023

anonrig commented Sep 24, 2023

bnoordhuis Sep 25, 2023

anonrig Sep 25, 2023

bnoordhuis Sep 26, 2023

nodejs-github-bot commented Sep 25, 2023

targos commented Sep 26, 2023

fs: improve readFileSync with file descriptors #49691

fs: improve readFileSync with file descriptors #49691

Conversation

anonrig commented Sep 17, 2023 • edited Loading

rluvaton commented Sep 18, 2023

bnoordhuis commented Sep 18, 2023

anonrig commented Sep 18, 2023

bnoordhuis commented Sep 18, 2023

anonrig commented Sep 18, 2023

anonrig commented Sep 24, 2023 • edited by targos Loading

nodejs-github-bot commented Sep 24, 2023

anonrig commented Sep 24, 2023

bnoordhuis Sep 25, 2023

Choose a reason for hiding this comment

anonrig Sep 25, 2023

Choose a reason for hiding this comment

bnoordhuis Sep 26, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 25, 2023

targos commented Sep 26, 2023

fs: improve `readFileSync` with file descriptors #49691

fs: improve `readFileSync` with file descriptors #49691

anonrig commented Sep 17, 2023 •

edited

Loading

anonrig commented Sep 24, 2023 •

edited by targos

Loading