src,http2: use native-layer pipe mechanism from FileHandle instead of synchronous I/O
Hi everyone! :)
This PR resolves the issue of using synchronous I/O for file-backed HTTP/2 streams, by turning FileHandle
s into (currently read-only) StreamBase
instances, adding a generic mechanism to pipe from a StreamBase
into another StreamBase
(where currently the latter one needs to know that it wants to have data written to it, which is true for HTTP/2 streams).
This could probably land as it is, but I’ve marked it as WIP because the code probably isn’t quite documented yet and I want to investigate more into the performance of these changes:
The http2/respond-with-fd
benchmark shows a ~30 % (update: down to more, like, 20 %) performance drop for me. At least part of that is likely related to the way the benchmark is written: It performs tens or hundreds of reads for the same portion of the same short file in order to return data to the client. In particular, the kernel will always immediately have the data available and will never need to perform actual disk operations, so synchronous reads actually come with less overhead than asynchronous reads to begin with.
In some way, that’s realistic: A lot of the time, data will already be in the kernel cache. On the other hand, Node has committed to avoiding synchronous I/O, and we can’t really know whether disk I/O will be slow or fast in advance.
(A lot of these assumptions are based on the fact than when I add a nanosleep()
call with 1 ns duration – which obviously takes longer than 1 ns, but it appears to be enough – to the pread64()
syscall, the 30 % drop turns into a whopping 100 % perf win).
In any case, I’d love to hear ideas and suggestions, both about the general approach and its performance here.
/cc @nodejs/http2 @jasnell @mcollina @apapirovski @Fishrock123
Commits:
-
http2: simplify timeout tracking
There’s no need to reset the chunk counter for every write.
-
src: make
FileHandle
a (readonly)StreamBase
This enables accessing files using a more standard pattern.
Once some more refactoring has been performed on the other existing
StreamBase
streams, this could also be used to implementfs
streams in a more standard manner. -
src: give StreamBases the capability to ask for data
Add a
OnStreamWantsWrite()
event that allows streams to ask for more input data if they want some. -
src: introduce native-layer stream piping
Provide a way to create pipes between native
StreamBase
instances that acts more directly than a.pipe()
call would. -
http2: use native pipe instead of synchronous I/O
This resolves the issue of using synchronous I/O for
respondWithFile()
andrespondWithFD()
, and enables scenarios in which the underlying file does not need to be a regular file. -
http2: remove regular-file-only restriction
Requiring
respondWithFile()
to only work with regular files is an artificial restriction on Node’s side and has become unnecessary.Offsets or lengths cannot be specified for those files, but that is an inherent property of other file types.
Checklist
-
make -j4 test
(UNIX), orvcbuild test
(Windows) passes -
tests and/or benchmarks are included -
documentation is changed or added -
commit message follows commit guidelines
Affected core subsystem(s)
src,http2
CI: https://ci.nodejs.org/job/node-test-commit/16439/
CI: https://ci.nodejs.org/job/node-test-commit/16441/
CI: https://ci.nodejs.org/job/node-test-commit/16447/