GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||
GeezerVison is showing github.com — fs: improve promise based readFile performance for big files by BridgeAR · Pull Request #44295 · nodejs/node Conversation
Member
This significantly reduces the peak memory for the promise Refs: https://github.com/nodejs/node/discussions/44239#discussioncomment-3428693 Signed-off-by: Ruben Bridgewater [email protected] Benchmark with a few runs:
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
mcollina
approved these changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lgtm
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
BridgeAR
marked this pull request as draft
BridgeAR
force-pushed
the
improve-fs-promises-readfile
branch
from
744bbd9 to
e72802b
Compare
BridgeAR
marked this pull request as ready for review
|
There was an error while loading. Please reload this page.
|
We might consider a similar fix for #41435. If I checked correct, it outperforms the regular readFile variant for big files. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Does this also impact "old" callback- based readFile? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
No. This is solely about the promise based readFile with encodings. The non encoding part is also not impacted. I ran the benchmark on our machines and there seems to be a weird effect for some file sizes where there's a performance drop. I guess it has to do with how V8 handles some things internally. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1176/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@BridgeAR are you planning to do the same for the sync and callback versions too? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
3f028d6 to
83562ca
Compare
This significantly reduces the peak memory for the promise based readFile operation by reusing a single memory chunk after each read and strinigifying that chunk immediately. Signed-off-by: Ruben Bridgewater <[email protected]>
4d4997b to
6c92996
Compare
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@mcollina I suggest to merge this and I am looking into the other part afterwards. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yes, let's do it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Commit Queue failed- Loading data for nodejs/node/pull/44295 ✔ Done loading data for nodejs/node/pull/44295 ----------------------------------- PR info ------------------------------------ Title fs: improve promise based readFile performance for big files (#44295) Author Ruben Bridgewater (@BridgeAR) Branch BridgeAR:improve-fs-promises-readfile -> nodejs:main Labels fs Commits 1 - fs: improve promise based readFile performance for big files Committers 1 - Ruben Bridgewater PR-URL: https://github.com/nodejs/node/pull/44295 Reviewed-By: Matteo Collina Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/44295 Reviewed-By: Matteo Collina Reviewed-By: James M Snell -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - fs: improve promise based readFile performance for big files ℹ This PR was created on Fri, 19 Aug 2022 15:12:52 GMT ✔ Approvals: 2 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/44295#pullrequestreview-1080779016 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/44295#pullrequestreview-1087869731 ✔ Last GitHub CI successful ℹ Last Benchmark CI on 2022-08-20T10:00:08Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1176/ ℹ Last Full PR CI on 2022-10-06T13:18:58Z: https://ci.nodejs.org/job/node-test-pull-request/47101/ - Querying data for job/node-test-pull-request/47101/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/3198926242 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in d3dd49f |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
This significantly reduces the peak memory for the promise based readFile operation by reusing a single memory chunk after each read and strinigifying that chunk immediately. Signed-off-by: Ruben Bridgewater <[email protected]> PR-URL: #44295 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.