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.

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

Reduce allocs in HTTP2StreamChannel #449

Merged
merged 5 commits into from
Jul 18, 2024
Merged

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Jul 12, 2024

Motivation:

The HTTP2StreamChannel stores two atomic booleans for whether the channel is active and writable. These are only accessed externally and access is relatively infrequent. Each atomic requires an allocation, instead we can just protect each with a single lock.

Modifications:

  • Combine the two atomics into a single 'flags' struct protected by a locked value box

Result:

Fewer alloations

@glbrntt glbrntt marked this pull request as ready for review July 12, 2024 16:59
@glbrntt glbrntt requested a review from FranzBusch July 12, 2024 17:00
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jul 12, 2024
@Lukasa
Copy link
Contributor

Lukasa commented Jul 15, 2024

It would be useful to get an idea of whether there's any significant performance impact to this change. I suspect that for most applications it comes out about neutral, but I'm not sure.

@glbrntt
Copy link
Contributor Author

glbrntt commented Jul 15, 2024

It would be useful to get an idea of whether there's any significant performance impact to this change. I suspect that for most applications it comes out about neutral, but I'm not sure.

This will obviously depend on how frequently isActive/isWritable are called. My gut feeling is that most users rarely call these.

A concrete example is AHC which uses it a few times per request (in channel active and when writability changes). I used it to benchmark against httpbin (running locally) and the difference was negligible.

@gjcairo
Copy link
Contributor

gjcairo commented Jul 17, 2024

You'll need to update the allocations for main too

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

As the change is low risk and easily reverted, I'm fine with it.

glbrntt added 4 commits July 18, 2024 08:46
Motivation:

The HTTP2StreamChannel stores two atomic booleans for whether the
channel is active and writable. These are only accessed externally and
access is relatively infrequent. Each atomic requires an allocation,
instead we can just protect each with a single lock.

Modifications:

- Combine the two atomics into a single 'flags' struct protected by a
  locked value box

Result:

Fewer alloations
@glbrntt glbrntt enabled auto-merge (squash) July 18, 2024 07:48
@glbrntt glbrntt merged commit ab04d05 into apple:main Jul 18, 2024
6 of 7 checks passed
@glbrntt glbrntt deleted the merge-atomics branch July 18, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants