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

Reduce allocations on InlineStreamMultiplexer/createStreamChannel #450

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Reduce allocations on InlineStreamMultiplexer/createStreamChannel #450

merged 2 commits into from
Jul 16, 2024

Conversation

gjcairo
Copy link
Contributor

@gjcairo gjcairo commented Jul 12, 2024 •
edited
Loading

Motivation

When creating stream channels on the InlineStreamMultiplexer, we are currently performing a flatSubmit on two different codepaths. This allocates two ELFs: one on submit, and one on flatMap. This is unnecessary, as we could do with just one allocation.

Modifications

Create a single ELP and call execute on the EL instead of using flatSubmit.

Result

Fewer allocations

@glbrntt
Copy link
Contributor

glbrntt commented Jul 12, 2024

@swift-server-bot add to allowlist

@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jul 12, 2024
@gjcairo gjcairo changed the title Reduce allocations on InlineStreamMultiplexer/createStreamChannel Reduce allocations on InlineStreamMultiplexer/createStreamChannel Jul 12, 2024
@gjcairo gjcairo force-pushed the create-stream-channel-allocs branch from d237287 to c198a0f Compare July 15, 2024 11:42
@gjcairo
Copy link
Contributor Author

gjcairo commented Jul 15, 2024

@swift-server-bot test this please

@gjcairo gjcairo force-pushed the create-stream-channel-allocs branch from c198a0f to 763e0ce Compare July 15, 2024 11:51
glbrntt
glbrntt requested changes Jul 15, 2024
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.

Looks good apart from fixing the warnings!

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated
@@ -173,12 +173,13 @@ extension Channel {
)
}
} else {
let unsafePosition = UnsafeTransfer(position)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be doing this: position isn't sendable because it can contain a ChannelHandler which might not be Sendable. All we're doing here is suppressing a correct warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah this is not supposed to stay :) that's why this was just opened as draft. I just wanted to get the nightly pipeline to not fatal error (it's got warnings as errors enabled so it's been failing because of this for a while now) to double check the tests passed. But all of the pipelines seem broken right now anyways because of some Jenkins error I believe so I'll just revert this and trust in my local builds passing.

We need to fix this somehow though, or remove the warns as errors flag temporarily.

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated Show resolved Hide resolved
@gjcairo gjcairo force-pushed the create-stream-channel-allocs branch from 763e0ce to 39711e2 Compare July 15, 2024 13:17
@gjcairo
Copy link
Contributor Author

gjcairo commented Jul 15, 2024

I updated the allocations for nightly because they had been outdated for a while.

However this change doesn't affect any allocation tests: this is because allocation tests that call createStreamChannel, call the createStreamChannel(promise:_:) version (instead of createStreamChannel(_:)), which is unaffected by this change. We could duplicate the allocation tests to use createStreamChannel(_:), or modify them to stop using the version with the promise, but I don't know if that's worth it/desired?

@gjcairo gjcairo requested a review from glbrntt July 15, 2024 14:06
@gjcairo gjcairo marked this pull request as ready for review July 15, 2024 14:06
@glbrntt
Copy link
Contributor

glbrntt commented Jul 15, 2024

However this change doesn't affect any allocation tests: this is because allocation tests that call createStreamChannel, call the createStreamChannel(promise:_:) version (instead of createStreamChannel(_:)), which is unaffected by this change. We could duplicate the allocation tests to use createStreamChannel(_:), or modify them to stop using the version with the promise, but I don't know if that's worth it/desired?

I think it's probably worth adding a version of the test which doesn't pass in the promise in a separate PR and verifying that we do save allocations here.

@gjcairo gjcairo force-pushed the create-stream-channel-allocs branch from 67812be to 3481691 Compare July 16, 2024 14:19
gjcairo
gjcairo commented Jul 16, 2024
View reviewed changes
docker/docker-compose.2204.main.yaml
@@ -39,7 +39,7 @@ services:
- MAX_ALLOCS_ALLOWED_client_server_request_response_many_inline=889050
- MAX_ALLOCS_ALLOWED_create_client_stream_channel=37050
- MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline=37050
- MAX_ALLOCS_ALLOWED_create_client_stream_channel_inline_no_promise_based_API=44050
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the allocations of the newly added test in the latest commit

glbrntt
glbrntt approved these changes Jul 16, 2024
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!

@glbrntt glbrntt enabled auto-merge (squash) July 16, 2024 14:21
@glbrntt glbrntt merged commit 79802e0 into apple:main Jul 16, 2024
6 of 7 checks passed
@gjcairo gjcairo deleted the create-stream-channel-allocs branch July 16, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glbrntt glbrntt glbrntt approved these changes

Assignees
No one assigned
Labels
🔨 semver/patch No public API change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gjcairo @glbrntt