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

Make HEADERS frame payload non-indirect #428

Closed Lukasa closed 2 months ago

Lukasa commented 10 months ago

Motivation:

In previous patches we shrank the size of HTTP2Frame by making various data types indirect in the frame payload. This included HEADERS, which is unfortunte as HEADERS frames are quite common.

This patch changes the layout of the HEADERS frame to remove the indirect case.

Modifications:

Result:

HEADERS frames are cheaper.

dnadoba commented 10 months ago

Can we add a unit test that checks that the size of FramePayload/HTTP2Frame doesn't exceed the maximum size of the inline existential storage? IIRC that would be 3 words / 24 Bytes on 64-bit systems.

Lukasa commented 10 months ago

Done.

dnadoba commented 10 months ago

The CI failure is interesting:

13:31:41 /code/Sources/NIOHTTP2/HTTP2Frame.swift:257:17: error: stored property 'booleans' of 'Sendable'-conforming struct 'Headers' has non-sendable type 'HTTP2Frame.FramePayload.Headers.Booleans'
13:31:41             var booleans: Booleans
13:31:41                 ^
13:31:41 /code/Sources/NIOHTTP2/HTTP2Frame.swift:226:20: note: consider making struct 'Booleans' conform to the 'Sendable' protocol
13:31:41             struct Booleans: OptionSet {
13:31:41                    ^
13:31:41                                       , Sendable

Booleans is an internal struct which is just made up of a single stored property which is Sendable. The compiler should be able to automatically synthesise Sendable conformance for us. Something is wrong here. Maybe @usableFromInline or the nesting is screwing with the compiler inference.

Lukasa commented 10 months ago

Updated.

dnadoba commented 10 months ago

Now we are hitting depreciation warnings because of the new NIO release:

15:38:56 /code/Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift:174:25: error: 'init(synchronouslyWrapping:configuration:)' is deprecated: This method has been deprecated since it defaults to deinit based resource teardown
15:38:56                     try NIOAsyncChannel(
15:38:56                         ^
15:38:56 /code/Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift:174:25: note: use 'init(wrappingChannelSynchronously:configuration:)' instead
15:38:56                     try NIOAsyncChannel(
15:38:56                         ^
15:38:56 /code/Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift:192:66: error: 'inbound' is deprecated: Use the executeThenClose scoped method instead.
15:38:56                     for try await receivedFrame in streamChannel.inbound {
15:38:56                                                                  ^
15:38:56 /code/Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift:195:49: error: 'outbound' is deprecated: Use the executeThenClose scoped method instead.
15:38:56                         try await streamChannel.outbound.write(ConfiguringPipelineAsyncMultiplexerTests.responseFramePayload)
dnadoba commented 10 months ago

I have reported the Sendable issue: https://github.com/apple/swift/issues/70019