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

Coalesce flushes during the read cycle. #132

Merged
merged 2 commits into from
May 24, 2019

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented May 24, 2019

Motivation:

The HTTP2StreamMultiplexer currently passes frames from child streams
through to the rest of the pipeline, and does the same with flushes.
Because HTTP2 is multiplexed, it is highly likely that in any
channelRead cycle multiple child channels will make sufficient progress
to want to flush their results. This means we can end up with multiple,
redundant, flush calls.

It would be better in general to try to reduce the amount of flushing
necessary to no more than once per channelRead cycle.

Modifications:

  • Keep track of the channelRead cycle and where we are in it.
  • Avoid flushing when we can expect a channelReadComplete.
  • Flush on channelReadComplete if necessary.

Result:

Fewer flushes, better performance under high read load.

Motivation:

The HTTP2StreamMultiplexer currently passes frames from child streams
through to the rest of the pipeline, and does the same with flushes.
Because HTTP2 is multiplexed, it is highly likely that in any
channelRead cycle multiple child channels will make sufficient progress
to want to flush their results. This means we can end up with multiple,
redundant, flush calls.

It would be better in general to try to reduce the amount of flushing
necessary to no more than once per channelRead cycle.

Modifications:

- Keep track of the channelRead cycle and where we are in it.
- Avoid flushing when we can expect a channelReadComplete.
- Flush on channelReadComplete if necessary.

Result:

Fewer flushes, better performance under high read load.
@Lukasa Lukasa added the semver/patch No public API change. label May 24, 2019
@Lukasa Lukasa added this to the 1.2.2 milestone May 24, 2019
@Lukasa Lukasa requested a review from weissi May 24, 2019 16:38
@Lukasa
Copy link
Contributor Author

Lukasa commented May 24, 2019

Transient test failure due to HPACK perf test.

@Lukasa
Copy link
Contributor Author

Lukasa commented May 24, 2019

@swift-nio-bot test this please

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

great! looks good :)

@Lukasa
Copy link
Contributor Author

Lukasa commented May 24, 2019

@swift-nio-bot test this please

@weissi weissi merged commit 0d153b5 into apple:master May 24, 2019
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.

2 participants