Save slicing HTTP 2 headers & data

[franz1981]

Motivation:

DefaultHttp2FrameWriter's always create aggregate promises and slices out of headers and data: both could be saved while reducing the amount of pipeline traversals in case the additional cost of creating a sliced buffer surpass the data to be written.

Modifications:

Small header and data could be copied directly in a single buffer, without any need to create aggregate promises.

Result:

Faster small data's writes

[franz1981]

@normanmaurer I'm not quite sure i could reuse the promise, so please take a look if I'm not assuming anything wrong...

The cutoff value has been decided by summing the required bytes for both a new sliced buffer (around 40 bytes ) + outbound buffer's entries (64 or 64 * 2 bytes, depending if headers/data paths) + promise aggregator (64 bytes), which means > 128 bytes.
Choosing 128 as a cutoff value has been conservative but able to capture many existing cases.
@idelpivnitskiy are you aware if this is something useful beyond microbenchmark, in term of cut-off values?

From what I can see, encoded response headers below 128 bytes are quite common, while for the data, depends, but it is still a possible scenario, although more typical in benchmarks IMO.

[franz1981]

This is just a preliminary example of the improvement, given that it doesn't account for the saving of the aggregate promise (which translate in less morphism of call-sites around the promises handling):

before:

Benchmark                                (padding)  (payloadSize)  (pooled)  Mode  Cnt    Score   Error  Units
Http2FrameWriterDataBenchmark.newWriter          0             64      true  avgt   10  112.406 ± 1.096  ns/op
Http2FrameWriterDataBenchmark.newWriter          0             64     false  avgt   10  110.992 ± 0.120  ns/op
Http2FrameWriterDataBenchmark.newWriter          0           1024      true  avgt   10  102.480 ± 1.899  ns/op
Http2FrameWriterDataBenchmark.newWriter          0           1024     false  avgt   10  112.736 ± 5.971  ns/op

now:

Benchmark                                (padding)  (payloadSize)  (pooled)  Mode  Cnt    Score   Error  Units
Http2FrameWriterDataBenchmark.newWriter          0             64      true  avgt   10   91.973 ± 0.343  ns/op
Http2FrameWriterDataBenchmark.newWriter          0             64     false  avgt   10  100.445 ± 0.238  ns/op
Http2FrameWriterDataBenchmark.newWriter          0           1024      true  avgt   10   94.347 ± 0.157  ns/op
Http2FrameWriterDataBenchmark.newWriter          0           1024     false  avgt   10   98.489 ± 0.196  ns/op

which is already ~10-20% improvement just on headers/data writing.

[franz1981]

An additional benefit (specific to Vertx, but can really happen regardless), is that the data written by users could uses heap-based ByteBuf and embedding earlier their content into the direct buffer to send would save performing the copy, see

this code path just disappear with this PR, given that no heap buffer is passed to the transport.

I've added 003d1e29350c47fe50d40456312b25d5e91720da to ignore the cutoff limits if the data is held in an array buffer, so we would perform the copy earlier, saving creating the aggregate promise and the additional pipeline traversal

[franz1981]

I think I have a leak when the data buffer is not sent anymore across the transport, and be released after flushed. I should release it right after copied

[franz1981]

@ejona86 @carl-mastrangelo do you have any chance (or any pointer to make me try it) to verify/validate some of the changes I am sending recently, performance-wise? I'm thinking about gRpc over HTTP 2 and having something different from our Quarkus/Vertx stack would help a lot

[franz1981]

@bryce-anderson this is using a very similar optimization although I couldn't push it as far as http 1.1 and make it produce a single buffer overall (we don't even presize the header here :/)

[carl-mastrangelo]

Alas, I have moved on from the gRPC team, and don't have access to sample http2 traffic.

[franz1981]

Thanks @carl-mastrangelo to have answered; if there is some Google users interested into grpc Netty, please reach me out here

[normanmaurer]

@franz1981 Please let me know once this is ready again for review

[franz1981]

Yep, @normanmaurer this should be ready to go as well

[normanmaurer]

why re throw ? feels wrong

[franz1981]

Goog point; I've blindly copied from the original code this one iirc, but as you said, doesn't look right 👍

[franz1981]

I don't feel yet comfortable to change it yet, i see that while throwing the exception it allow to correctly release in the caller the expected "leaky" data. I would leave it as it is, unless you want me to address the original code behaviour now (which is fine, just need to know you opinion here)

[bryce-anderson]

Just a few comments that crossed my mind, I haven't fully looked through this yet.
The PromiseAggregator instances got me wondering: could this same goal be achieved using a CompositeByteBuf? Perhaps that is just passing the buck so to speak, but if the transport implementations are intelligently handling those types of buffers we can avoid the pipeline traversals easily.

[bryce-anderson]

Interesting. Can we use that down in writeHeadersPartsInternal as well?

[franz1981]

Yep, given that is very tiny implementation detail, I assumed to leverage on it just on the hottest path ie single buffer, no promise aggregation

[franz1981]

@bryce-anderson

could this same goal be achieved using a CompositeByteBuf

That will both help with traversal, saving aggregate promises and saving entries on the outbound buffer, which is something I would explore regardless but, I still believe it tries to leverage writev on the transport, which is a shame to me (@normanmaurer am I right? ) when components are such small.
To solve/improve this we could think (unless already exists, but I don't think so) about an opt-in mechanism on the transport to avoid using writev with many small sized buffers (could be either from composite or others buffer types) cause is not as efficient as it could.

While what this pr deliver is another additional improvement (without introducing new buffer types); it immediately release the buffer which contains the data, embedding its content, making it available again to be re-used and saving the inherent additional heap cost of allocating the slice, amortizing it in the buffer which embedd it.

[franz1981]

PTAL @bryce-anderson @normanmaurer

Related the suggestion to use CompositeByteBuf I would like to talk about this in a separate issue, given that it is something I wished to do in long time (together to some smart aggregation of small composite buffers), but requires some attention given how epoll works ie iovec max prevent sending in one go a direct composite buffer with "too many" components.
Not only, in the current moment I don't think composite buffers to work well with tiny buffers: every transport translate composite buffers in writev-like syscalls, which I much doubt are efficient with too small buffers.
Moreover, specifically with epoll; I have replaced one year ago write with send syscalls because the former was slightly more costly and I think the same applies to writev, compared to send

[franz1981]

@normanmaurer I see that the original code was throwing exceptions, while not expected AND not correctly releasing multiple retained header buffers; I could try fix the former but the latter would requires a separate pr really, given that seems a much more annoying and complex technical debt, which didn't yet bitten us.

[franz1981]

mmm @normanmaurer what happened for 33f5c56? :P

[normanmaurer]

@franz1981 what you mean ?

[franz1981]

@normanmaurer I've received notification of Merge branch '4.1' into 4.1_no_slice changes, but IDK what it is :P

[normanmaurer]

@franz1981 ah this was just to bring it up to date with current 4.1

[franz1981]

@normanmaurer there is something more you want me to take a look for this before getting another review round?

As said, I am not very confident the original code was correctly handling leaks here, but is really tons of code and we should address it separately (eg all the header retained slices around, the thrown you noticed in some earlier comment) - ideally first, if we think them to be critic.

What I could do is to try address them already for the fast-paths I've introduced, but will create the weird situation, where the optimized (although common enough, I suppose) new stuff is more correct than what exists from some time already...

[normanmaurer]

@franz1981 just hold of a bit... I will try to find time to review this one first

[franz1981]

@bryce-anderson if you want to give it another shot, I have tried implementing your suggestion to further reduce the number of allocations

[franz1981]

Any other concerns @bryce-anderson or @normanmaurer ?
I can try reducing the amount of changes if it helps

[franz1981]

@normanmaurer @chrisvest any news for this folks?
Not urgent but the performance effects was rather positives....

[normanmaurer]

@ejona86 @idelpivnitskiy @bryce-anderson can you check again ?