Skip to content

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 FileHandles 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 implement fs 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() and respondWithFD(), 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), or vcbuild 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/

Merge request reports

Loading