src,http2: use native-layer pipe mechanism from FileHandle instead of synchronous I/O

[addaleax]

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/

[fhinkel]

Why not std::unique_ptr?

std::unique_ptr<FileHandleReadWrap> req_wrap(FileHandleReadWrap::from_req(req));

The delete then happens automatically at the end of the block.

[addaleax]

@fhinkel Done! But mostly because moving towards always using smart pointers is good. In this case, I am not sure that it actually makes the code or ownership flow cleaner (so I’ve added a comment for that).

[mcollina]

IMHO files served in this way would always be in the kernel cache, and this is a very tiny read.

Can you add a realistic benchmark, ideally in another PR so that we can use it as a baseline to compare the two implementations?

Can you generate a flamegraph of before/after the change? It would be useful to understand the performance impact.

[mcollina]

Is creating a function here ok from a performance point of view? This is not the typical style I've used when working with libuv.

[addaleax]

@mcollina This doesn’t create a new function in C++.

This is not the typical style I've used when working with libuv.

I think that’s just because a lot of our code that deals with libuv was written before we started using C++11.

[addaleax]

IMHO files served in this way would always be in the kernel cache, and this is a very tiny read.

That would be true for a lot of cases, yes, and it’s something I’m worried about; because I’d expect that as long as files are in the kernel cache, synchronous I/O is always going to be faster than async I/O.

Can you add a realistic benchmark, ideally in another PR so that we can use it as a baseline to compare the two implementations?

Sure, but I’d be interested in the criteria we would use. I’m wondering whether something like a combination with the benchmarks that we have for network sockets would make sense here, i.e. sending one or more large files over multiple streams and requests in parallel, and measuring throughput rather than ops/sec.

Can you generate a flamegraph of before/after the change? It would be useful to understand the performance impact.

Tbh, I’m not sure what tooling would be best suited for that, but I’m sure I can try.

[fhinkel]

I would love to see more modern C++ in our libuv related code : 💓

[addaleax]

Flame graph comparisons:
Before: https://addaleax.net/h2p-flamegraph-before-1.html
After: https://addaleax.net/h2p-flamegraph-after-1.html

[jasnell]

Thank you for this. It's going to take me a few days to be able to get completely through this. I'll try to have some feedback by Monday :)

[apapirovski]

This looks great, @addaleax! In the middle of a major move so it's likely going to take me a little while to review.

[addaleax]

A 12% perf loss at 10k requests is a bit painful

Yeah. :/ Still, I think following the general principle of “never blocking the event loop” is enough to make this worth it. Also, there’s decent perf gains in other categories, and there’s always the fact that the benchmark doesn’t represent a real-world server and we’ll just have to see how this works out in reality.

I suspect that it's still significantly faster than using the normal fs.createReadStream() approach. Can you do a quick check to confirm? Even just local benchmarking should be enough.

Yeah, it is:

$ ./node benchmark/http2/respond-with-fd.js streams=200 requests=10000 
http2/respond-with-fd.js method="pipe" benchmarker="h2load" clients=1 streams=200 requests=10000: 3,375.29
http2/respond-with-fd.js method="respondwithfd" benchmarker="h2load" clients=1 streams=200 requests=10000: 6,490.17
http2/respond-with-fd.js method="pipe" benchmarker="h2load" clients=2 streams=200 requests=10000: 3,642.84
http2/respond-with-fd.js method="respondwithfd" benchmarker="h2load" clients=2 streams=200 requests=10000: 6,463.41

(Side note: I just about got the first prototype for a fast-path StreamBase-to-StreamBase .pipe() working & passing all existing tests – so soon there might not be much of a difference at all. 😄)

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/16925/

[addaleax]

Landed in 3ad7c1a...1ac1424

[targos]

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

[addaleax]

This depends on #18297, so I don’t think we can backport it.