Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance of node:fs #106

Open
anonrig opened this issue Aug 18, 2023 · 19 comments · Fixed by nodejs/node#49593
Open

Performance of node:fs #106

anonrig opened this issue Aug 18, 2023 · 19 comments · Fixed by nodejs/node#49593
Assignees

Comments

@anonrig
Copy link
Member

anonrig commented Aug 18, 2023

node:fs is based on FSReqCallback and makes multiple C++ calls for any operation. There are lots of places where we could just open, stat and perform the operation using only 1 C++ call.

An example PR that provided performance boost was: nodejs/node#48658.

@anonrig
Copy link
Member Author

anonrig commented Sep 17, 2023

Sync methods

@tony-go
Copy link
Member

tony-go commented Sep 19, 2023

It's definitely a good idea to tackle syncs as we don't have to handle callbacks in c++ land as with async methods.

@CanadaHonk
Copy link
Member

CanadaHonk commented Sep 26, 2023

Would rimraf likely benefit from being rewritten in C++ rather than in JS using FS APIs? Potential issue/idea for the future.

@CanadaHonk
Copy link
Member

CanadaHonk commented Sep 26, 2023

Encoding C++ fast paths

utf8

ascii/latin1

ascii/latin1 allows using one byte V8 strings in/out which can save string copies and potentially lead to some speedups (needs investigation).

See also: nodejs/node#49888

@benjamingr
Copy link
Member

readfile intentionally splits the file into many reads to avoid contention but that could could live in C++ and readFile would could be a single JS->CPP call (and one CPP->JS for the callback)

@CanadaHonk
Copy link
Member

^ fwiw this only happens in current non-fast path atm (non-utf8 encoding)

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 27, 2023

Should we maybe put in the docs, that using a number instead of a string for mode is significantly faster?

nodejs/node#49756 (comment)

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 27, 2023

My benchmarks show that using a string for mode is about 12 Million ops/s and a number about 250 Million ops/s. My suggestion for string in the above comment makes it about 80 Million ops/s for but is still slower than simply to use a number.

@CanadaHonk
Copy link
Member

It could also be interesting to make a Node option/something which would log/warn when missing potential faster internal/FS paths (eg readFileSync utf8 encoding, or using a string as a mode, ...)

@aduh95
Copy link

aduh95 commented Sep 27, 2023

Should we maybe put in the docs, that using a number instead of a string for mode is significantly faster?

My two cents on this: we should avoid putting performance recommendation in there as it's likely to get outdated without us noticing. We could update the examples to use a constant if they are not already, so it nudges the reader to use the (currently) faster alternative.

@CanadaHonk
Copy link
Member

Related a bit, I noticed we use strings as default flag for some common FS ops, it might be a bit faster to use numbers instead?

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 27, 2023

There are two params. flag and mode. I just found test-fs-existssync-false.js where we use '0777' instead of 0o777. This could be changed to avoid the conversion bottleneck.

What do you refer to?

@CanadaHonk
Copy link
Member

CanadaHonk commented Sep 27, 2023

I meant flag not mode yes, readFileSync uses { flag: 'r' } as an example. I wonder if doing eg { flag: constants.O_RDONLY } would have a noticable change, but it might not as it only really uses a switch with the string which shouldn't have much of an impact.

nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Sep 27, 2023
PR-URL: #49856
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Nov 29, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Nov 29, 2023
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Nov 30, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Nov 30, 2023
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Dec 5, 2023
PR-URL: #50100
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Dec 11, 2023
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Dec 11, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Dec 12, 2023
PR-URL: #51075
Refs: nodejs/performance#106
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Dec 13, 2023
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Dec 13, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Dec 15, 2023
PR-URL: #50100
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Dec 15, 2023
PR-URL: #51075
Refs: nodejs/performance#106
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Dec 15, 2023
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Dec 15, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Dec 19, 2023
PR-URL: #49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Dec 19, 2023
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
UlisesGascon pushed a commit to nodejs/node that referenced this issue Jan 9, 2024
PR-URL: #49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@anonrig anonrig unpinned this issue Jan 22, 2024
richardlau pushed a commit to nodejs/node that referenced this issue Mar 25, 2024
PR-URL: #50100
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
richardlau pushed a commit to nodejs/node that referenced this issue Mar 25, 2024
PR-URL: #51075
Refs: nodejs/performance#106
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
PR-URL: nodejs#49856
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants