search

apple / swift-nio-http2

HTTP/2 support for SwiftNIO
https://swiftpackageindex.com/apple/swift-nio-http2/main/documentation/niohttp2
Apache License 2.0
462 stars 82 forks source link

Inline multiplexer calls writability mgr directly #386

Closed rnro closed 1 year ago

rnro commented 1 year ago

Motivation:

The HTTP2StreamChannel keeps track of how many bytes have been written and how many are buffered for use in evaluating the 'writability' of the stream channel. Bytes are considered 'written' or at least no longer buffered when the write is completed (successfully or errored). In order to make the determination of when writes are complete across channel handlers we attach a promise to all writes and deduct adjust the byte counts on completion. Creating and attaching these promises requires costly allocations.

Modifications:

We can remove the need to create these promises for the new inline stream multiplexer because the multiplexer and man HTTP2 channel operations are now all within the HTTP2ChannelHandler. This change calls the writabilityManager directly when writes are completed.

Result:

rnro commented 1 year ago

This change reduces the allocations for the test_client_server_request_response many (1000) requests allocation test from 1.3M to 1.0M.

Lukasa commented 1 year ago

It should be noted that the 9 byte frame header does not count against the flow control windows, so we do not need to consider it here.

RFC 9113 § 6.9.1:

For flow-control calculations, the 9-octet frame header is not counted.

glbrntt commented 1 year ago

It should be noted that the 9 byte frame header does not count against the flow control windows, so we do not need to consider it here.

RFC 9113 § 6.9.1:

For flow-control calculations, the 9-octet frame header is not counted.

Oh great! We just need to update estimatedFrameSize or the value we provide to wroteBytes to ignore that then.

Lukasa commented 1 year ago

Yeah I think the fix is to drop the use of estimatedFrameSize altogether, tbh. I don't really know what I was thinking when I proposed it. Might be worth writing a separate patch to replace this with a flowControlledSize that correctly only covers that question.

rnro commented 1 year ago

I've opened a PR for changing that estimatedFrameSize https://github.com/apple/swift-nio-http2/pull/387