swift-nio-http2 icon indicating copy to clipboard operation
swift-nio-http2 copied to clipboard

Make HEADERS frame payload non-indirect

Open Lukasa opened this issue 1 year ago • 6 comments

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:

  • Move END_STREAM into an OptionSet.
  • Turn the two optional bits into flags in the aforementioned OptionSet
  • Replace the properties with computed properties.
  • Remove the indirect case

Result:

HEADERS frames are cheaper.

Lukasa avatar Nov 16 '23 13:11 Lukasa

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.

dnadoba avatar Nov 16 '23 13:11 dnadoba

Done.

Lukasa avatar Nov 16 '23 13:11 Lukasa

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.

dnadoba avatar Nov 16 '23 13:11 dnadoba

Updated.

Lukasa avatar Nov 17 '23 15:11 Lukasa

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 avatar Nov 17 '23 17:11 dnadoba

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

dnadoba avatar Nov 24 '23 14:11 dnadoba