Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make HEADERS frame payload non-indirect #428

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make HEADERS frame payload non-indirect #428

wants to merge 2 commits into from

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Nov 16, 2023

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 Lukasa added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Nov 16, 2023
@dnadoba
Copy link
Member

dnadoba commented Nov 16, 2023

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
Copy link
Contributor Author

Lukasa commented Nov 16, 2023

Done.

dnadoba
dnadoba approved these changes Nov 16, 2023
View reviewed changes
Copy link
Member

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great performance/allocation improvement!

glbrntt
glbrntt approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! ⛳️

Sources/NIOHTTP2/HTTP2Frame.swift Outdated
Comment on lines 34 to 38
internal init(exclusive: Bool, dependency: HTTP2StreamID, weight: UInt8) {
self.exclusive = exclusive
self.dependency = dependency
self.weight = weight
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the init intentionally internal before?

It's a parameter in Headers.init but is defaulted to nil so it looks like it not being public was an oversight?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an oversight to me as well

@dnadoba
Copy link
Member

dnadoba commented Nov 16, 2023

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
Make HEADERS frame payload non-indirect
b42fbe5 
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 Lukasa added 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 and removed patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) labels Nov 17, 2023
@Lukasa
Copy link
Contributor Author

Lukasa commented Nov 17, 2023

Updated.

@dnadoba
Copy link
Member

dnadoba commented Nov 17, 2023

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
Copy link
Member

dnadoba commented Nov 24, 2023

I have reported the Sendable issue: apple/swift#70019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnadoba dnadoba dnadoba approved these changes

@glbrntt glbrntt glbrntt approved these changes

Assignees
No one assigned
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Lukasa @dnadoba @glbrntt