Flow of fs.readFile and suggest improvements

[anonrig]

I'm trying to understand fs.readFile's flow and understand where the bottlenecks are. Since I'm quite new to Node.js (especially on the C++ side), I wanted to create this discussion to receive some feedback about potential improvements that can be made for fs.readFile and fs.readFileSync.

Flow

• readFile
  • Implemented in https://github.com/nodejs/node/blob/main/lib/fs.js#L387
  • create ReadFileContext
  • FSReqCallback with oncomplete readFileAfterOpen
    • Make sure we have the proper access to the file
  • Calls binding.fs.open
• readFileAfterOpen
  • Implemented in https://github.com/nodejs/node/blob/main/lib/fs.js#L324
  • FSReqCallback with oncomplete readFileAfterStat
    • The size is required in the next step
  • calls binding.fstat
• readFileAfterStat
  • Implemented in https://github.com/nodejs/node/blob/main/lib/fs.js#L340
  • Gets the file size from binding.fstat
  • Set the proper buffer(s)
    • if size is 0
      • context.buffers = []
    • else
      • creates Buffer.allocUnsafeSlow with size
  • calls ReadFileContext.read()
• ReadFileContext.read()
  • Implemented in https://github.com/nodejs/node/blob/main/lib/internal/fs/read_file_context.js#L85
  • if size is 0
    • creates Buffer.allocUnsafeSlow with kReadFileUnknownBufferLength
  • create FSReqCallback with oncomplete readFileAfterRead
  • Calls internalBinding.fs.read
• ReadFileContext.readFileAfterRead
  • Implemented in https://github.com/nodejs/node/blob/main/lib/internal/fs/read_file_context.js#L25
  • Updates context position according to bytes read
  • If context position is context.size or bytes read is 0
    • close context
  • Calls ReadFileContext.read() again
• ReadFileContext.close
  • Implemented in https://github.com/nodejs/node/blob/main/lib/internal/fs/read_file_context.js#L112
  • Creates FSReqCallback with readFileAfterClose
  • Calls internalBinding.fs.close
• readFileAfterClose
  • Implemented in https://github.com/nodejs/node/blob/main/lib/internal/fs/read_file_context.js#L46
  • Returns context buffer according to encoding

Suggestions

1. Merge readFileAfterOpen and readFileAfterStat FSReqCallback.
   1. Will reduce the number of bridge (js/c++) communication
   2. Make sure we can open the file, and if we can open the file, then return the size
2. Call file close on c++ side of readFileAfterRead if the position of the read buffer is at the end.
   1. Will reduce the extra FSReqCallback
   2. readFileAfterClose will still be required if the read operation is aborted
3. Return data with proper encoding in ReadFileContext.readFileAfterRead
   1. No need to change the encoding of the buffer in readFileAfterClose
   2. Question: I don't know if we can encode the buffer chunk by chunk, and still have a valid output at the end

[anonrig]

Differences between Bun and Node.js

• Bun calls fstat in Zig on readFile.
  • Implemented in https://github.com/oven-sh/bun/blob/01f567e91541ffbbc7722c4dc7111d27d9923838/src/bun.js/node/node_fs.zig#L3332
  • There is no extra communication between JS and Zig.
• Bun does not send the data between Zig and JS chunk by chunk
  • Implemented in https://github.com/oven-sh/bun/blob/01f567e91541ffbbc7722c4dc7111d27d9923838/src/bun.js/node/node_fs.zig#L3350
• Bun closes the file operation in the defer of the readFile
  • Implemented in https://github.com/oven-sh/bun/blob/01f567e91541ffbbc7722c4dc7111d27d9923838/src/bun.js/node/node_fs.zig#L3329

[mcollina]

The readFile(path, ‘utf8’, cb) is the main I/O bottleneck of all bundlers and it should be specifically optimized (possibly the sync one too). Currently it’s incredibly inefficient as it reads all data as buffers, concatenate them, and then convert it to string. This allocates 3x the data of the file to read.

Another significant angle to pursue is to use better kernel primitives for file system operations, e.g. finishing libuv/libuv#1947.

[BridgeAR]

it reads all data as buffers, concatenate them, and then convert it to string

The conat is only done in case the file size can not be determined. That case can be optimized by directly converting the buffer to a string with StringDecoder and to concat those. But that should not be the common case.
The same logic could however also be used for chunked reads. In that case we only allocate a single chunk, convert that to a string and reuse the chunk for the next read. That way the total memory allocated would be the file size plus one chunk = ~one allocation.

[BridgeAR]

I just implemented this locally and the benchmark looks quite solid:

                                                              confidence improvement accuracy (*)   (**)  (***)
fs/readfile-promises.js concurrent=1 len=1024 duration=5                     -2.37 %       ±3.82% ±5.09% ±6.62%
fs/readfile-promises.js concurrent=1 len=33554432 duration=5         ***     53.27 %       ±3.07% ±4.08% ±5.32%
fs/readfile-promises.js concurrent=10 len=1024 duration=5            ***      7.67 %       ±4.18% ±5.56% ±7.24%
fs/readfile-promises.js concurrent=10 len=33554432 duration=5        ***     18.27 %       ±2.73% ±3.64% ±4.73%

I benchmarked against v16 and that might be the reason that the small buffers are improved as well as they should be roughly on par (I have a somewhat simpler implementation but that should not have a strong effect).

[mcollina]

PR? Let's ship this as it should speed up webpack.

[anonrig]

@BridgeAR can you open a pull request for this changes?

[BridgeAR]

It's already merged. I did not yet open a PR to do something similar with the callback based one.

[tony-go]

hey here 👋

I started something here about merging open and fstat.