fs: improve fsPromises readFile performance by Linkgoron · Pull Request #37608 · nodejs/node

Linkgoron · 2021-03-05T11:33:48Z

Improve the fsPromises readFile performance by allocating only one buffer, when size is known. I also increased the size of the readbuffer chunks (which are now the same as in read_file_context), and also removed the extra read if size bytes have been read.

Most of the changes here are essentially shamelessly copied from read_file_context, I've also removed a call to the inner read and just called binding.read directly.

I also had to fix a few abort-controller tests, as they were dependant on the extra read at the end of the file.

This improves the promises.readFile performance by 20% on my machine. However there is still a 15-20% difference between the cb version and this version on my machine, so I didn't mark this as resolving #37583

edit:
second commit contains fs.promises.readFile benchmark

benjamingr · 2021-03-05T11:40:52Z

@nodejs/fs @jasnell

benjamingr

I went all the way back to https://github.com/nodejs/node/pull/18297/files to see if there is a compelling reason that buffer size was chosen - looks like it was tried initially and not really iterated on - so this change looks fine :)

nodejs-github-bot · 2021-03-05T12:29:30Z

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

Linkgoron · 2021-03-05T12:32:29Z

I went all the way back to https://github.com/nodejs/node/pull/18297/files to see if there is a compelling reason that buffer size was chosen - looks like it was tried initially and not really iterated on - so this change looks fine :)

@benjamingr I believe that the number was changed in read_file_context here #27063, but probably missed changing it as an oversight in the promisifed version.

benjamingr · 2021-03-05T12:41:24Z

cc @BridgeAR

nodejs-github-bot · 2021-03-05T14:06:16Z

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

RaisinTen · 2021-03-05T15:17:47Z

  readFile,
  writeFile,
-  truncate
+  truncate,


Suggested change

truncate,

truncate

nit: Unrelated change

Personally I'm in favour of not spending mental energy on stylistic changes that are not enforced as part of linting rules.

Linkgoron · 2021-03-05T16:58:10Z

I've copy-pasted the readfile benchmark and converted it to a promisified version and these are the results of the comparison tool:

                                                             confidence improvement accuracy (*)   (**)  (***)
fs/readfile-promises.jsconcurrent=1 len=1024 duration=5             ***     22.24 %       ±0.85% ±1.14% ±1.49%
fs/readfile-promises.jsconcurrent=1 len=16777216 duration=5         ***      9.93 %       ±0.82% ±1.09% ±1.43%
fs/readfile-promises.jsconcurrent=10 len=1024 duration=5            ***     25.43 %       ±0.86% ±1.14% ±1.49%
fs/readfile-promises.jsconcurrent=10 len=16777216 duration=5        ***     28.60 %       ±1.19% ±1.58% ±2.06%

Improve the fsPromises readFile performance by allocating only one buffer, when size is known, increase the size of the readbuffer chunks, and dont read more data if size bytes have been read refs: nodejs#37583

nodejs-github-bot · 2021-03-05T19:30:56Z

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

add a benchmark for fs.promises.readFile

nodejs-github-bot · 2021-03-05T19:32:55Z

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

addaleax · 2021-03-05T21:26:48Z

+      buffer = fullBuffer;
+      offset = totalRead;
+      length = MathMin(size - totalRead, kReadFileBufferLength);
+    }


Note that this is a behavioral change for files that are being appended to while they are being read. I think that should be okay, because ultimately there are no guarantees for the relative timing of the two operations, but it might be something to keep in mind.

Yes, you're correct and I should've mentioned it in the PR but forgot. This is aligning the behaviour with the cb version, so I thought that it would be OK.

node/lib/internal/fs/read_file_context.js

Line 110 in 26288ff

length = MathMin(kReadFileBufferLength, this.size - this.pos);

Yeah, I think that’s okay 👍

nodejs-github-bot · 2021-03-06T08:01:09Z

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

puzpuzpuz · 2021-03-10T13:39:00Z

-    const buf = Buffer.alloc(isFirstChunk ? firstChunkSize : chunkSize);
-    const { bytesRead, buffer } =
-      await read(filehandle, buf, 0, buf.length, -1);
-    endOfFile = bytesRead === 0;


nit: it looks like it should be enough to change this line to:

totalRead += bytesRead; endOfFile = bytesRead === 0 || totalRead === size;

in order to get the same performance improvement as in this PR (at least that's what I saw on my machine). Another useful optimization here is Buffer.allocUnsafeSlow. So, maybe it's worth considering keeping only these 4 lines of changes, but with the same result.

I agree that some of the changes are not strictly necessary for the improvement, but another feature (IMO) of my changes it that now the logic is essentially the same as the sync readFile.

benjamingr · 2021-03-10T16:01:24Z

Landed in e2f5bb7...b9fd4eb

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Mar 5, 2021

Linkgoron marked this pull request as ready for review March 5, 2021 11:39

benjamingr approved these changes Mar 5, 2021

View reviewed changes

Linkgoron mentioned this pull request Mar 5, 2021

fs: improve fsPromises writeFile performance #37610

Merged

benjamingr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Mar 5, 2021

jasnell approved these changes Mar 5, 2021

View reviewed changes

RaisinTen reviewed Mar 5, 2021

View reviewed changes

benjamingr added the needs-ci PRs that need a full CI run. label Mar 5, 2021

fs: improve fsPromises readFile performance

cfb1805

Improve the fsPromises readFile performance by allocating only one buffer, when size is known, increase the size of the readbuffer chunks, and dont read more data if size bytes have been read refs: nodejs#37583

Linkgoron force-pushed the fs-promises-improve-read-performance branch from ea69fbc to 1bbc2bb Compare March 5, 2021 19:28

benjamingr approved these changes Mar 5, 2021

View reviewed changes

fs: add promisified readFile benchmark

e31ccd9

add a benchmark for fs.promises.readFile

Linkgoron force-pushed the fs-promises-improve-read-performance branch from 1bbc2bb to e31ccd9 Compare March 5, 2021 19:31

benjamingr approved these changes Mar 5, 2021

View reviewed changes

addaleax reviewed Mar 5, 2021

View reviewed changes

addaleax approved these changes Mar 5, 2021

View reviewed changes

puzpuzpuz reviewed Mar 10, 2021

View reviewed changes

benjamingr added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 10, 2021

azu mentioned this pull request Mar 23, 2021

2021-03-23のJS: Node.js 15.12.0(fs.promisesのパフォーマンス改善)、strict CSP、CSS Auditing Tools jser/jser.github.io#846

Merged

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

MoritzLoewenstein mentioned this pull request Aug 21, 2021

[v14.x backport] fs: improve fsPromises readFile performance #39838

Closed

targos added backport-open-v14.x and removed backport-requested-v14.x labels Aug 22, 2021

targos added backported-to-v14.x and removed backport-open-v14.x labels Sep 4, 2021

targos mentioned this pull request Sep 4, 2021

v14.18.0 release proposal #39990

Merged

Uh oh!

Conversation

Linkgoron commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr commented Mar 5, 2021

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 5, 2021

Uh oh!

Linkgoron commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr commented Mar 5, 2021

Uh oh!

nodejs-github-bot commented Mar 5, 2021

Uh oh!

RaisinTen Mar 5, 2021 • 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!

benjamingr Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

Linkgoron commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 5, 2021

Uh oh!

nodejs-github-bot commented Mar 5, 2021

Uh oh!

addaleax Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

Linkgoron Mar 5, 2021 • 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!

addaleax Mar 6, 2021

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 6, 2021

Uh oh!

puzpuzpuz Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

Linkgoron Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Linkgoron commented Mar 5, 2021 •

edited

Loading

Linkgoron commented Mar 5, 2021 •

edited

Loading

RaisinTen Mar 5, 2021 •

edited

Loading

Linkgoron commented Mar 5, 2021 •

edited

Loading

Linkgoron Mar 5, 2021 •

edited

Loading