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

Respect HTTP/2 connection window size #1489

Merged
merged 3 commits into from Dec 11, 2018

Conversation

trustin
Copy link
Collaborator

@trustin trustin commented Dec 11, 2018

Motivation:

InboundTrafficController uses a hard-coded water mark value of 128KiB,
which is much smaller than our current initial connection window size
(1MiB).

Modifications:

  • Use the water mark values that matches the initial HTTP/2 connection
    window size.
    • HTTP/1 connections' water mark values stay same.
  • Disable Netty Channel write buffer water mark to reduce unnecessary
    emission of channelWritabilityChanged events.

Result:

  • Less frequent network read suspension which was unnecessary.
  • Less frequent channelWritabilityChanged events.

Motivation:

`InboundTrafficController` uses a hard-coded water mark value of 128KiB,
which is much smaller than our current initial connection window size
(1MiB).

Modifications:

- Use the water mark values that matches the initial HTTP/2 connection
  window size.
  - HTTP/1 connections' water mark values stay same.

Result:

- Less frequent network read suspension which was unnecessary.
@trustin trustin added the defect label Dec 11, 2018
@trustin trustin added this to the 0.77.0 milestone Dec 11, 2018
@codecov
Copy link

codecov bot commented Dec 11, 2018

Codecov Report

Merging #1489 into master will decrease coverage by 0.09%.
The diff coverage is 91.3%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #1489     +/-   ##
===========================================
- Coverage     72.26%   72.17%   -0.1%     
+ Complexity     6684     6675      -9     
===========================================
  Files           637      637             
  Lines         27667    27677     +10     
  Branches       3332     3332             
===========================================
- Hits          19994    19975     -19     
- Misses         5925     5949     +24     
- Partials       1748     1753      +5
Impacted Files Coverage Δ Complexity Δ
.../java/com/linecorp/armeria/client/HttpSession.java 46.66% <0%> (-3.34%) 5 <0> (ø)
.../linecorp/armeria/client/Http2ResponseDecoder.java 60.71% <100%> (+0.35%) 31 <1> (ø) ⬇️
...armeria/server/HttpServerPipelineConfigurator.java 77.07% <100%> (+0.11%) 12 <1> (ø) ⬇️
...m/linecorp/armeria/server/Http1RequestDecoder.java 75% <100%> (ø) 26 <0> (ø) ⬇️
...m/linecorp/armeria/client/HttpResponseDecoder.java 86.51% <100%> (ø) 16 <0> (ø) ⬇️
.../linecorp/armeria/client/Http1ResponseDecoder.java 60% <100%> (ø) 23 <0> (ø) ⬇️
...armeria/client/HttpClientPipelineConfigurator.java 70.37% <100%> (+0.1%) 36 <0> (ø) ⬇️
...p/armeria/client/Http2ClientConnectionHandler.java 82.35% <100%> (ø) 6 <0> (ø) ⬇️
...m/linecorp/armeria/server/Http2RequestDecoder.java 57.14% <100%> (+0.36%) 23 <0> (ø) ⬇️
...ava/com/linecorp/armeria/internal/ChannelUtil.java 91.3% <100%> (+0.82%) 8 <1> (+1) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 751ca56...715b37a. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Dec 11, 2018

Codecov Report

Merging #1489 into master will decrease coverage by 0.09%.
The diff coverage is 91.3%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #1489     +/-   ##
===========================================
- Coverage     72.26%   72.17%   -0.1%     
+ Complexity     6684     6675      -9     
===========================================
  Files           637      637             
  Lines         27667    27677     +10     
  Branches       3332     3332             
===========================================
- Hits          19994    19975     -19     
- Misses         5925     5949     +24     
- Partials       1748     1753      +5
Impacted Files Coverage Δ Complexity Δ
.../java/com/linecorp/armeria/client/HttpSession.java 46.66% <0%> (-3.34%) 5 <0> (ø)
.../linecorp/armeria/client/Http2ResponseDecoder.java 60.71% <100%> (+0.35%) 31 <1> (ø) ⬇️
...armeria/server/HttpServerPipelineConfigurator.java 77.07% <100%> (+0.11%) 12 <1> (ø) ⬇️
...m/linecorp/armeria/server/Http1RequestDecoder.java 75% <100%> (ø) 26 <0> (ø) ⬇️
...m/linecorp/armeria/client/HttpResponseDecoder.java 86.51% <100%> (ø) 16 <0> (ø) ⬇️
.../linecorp/armeria/client/Http1ResponseDecoder.java 60% <100%> (ø) 23 <0> (ø) ⬇️
...armeria/client/HttpClientPipelineConfigurator.java 70.37% <100%> (+0.1%) 36 <0> (ø) ⬇️
...p/armeria/client/Http2ClientConnectionHandler.java 82.35% <100%> (ø) 6 <0> (ø) ⬇️
...m/linecorp/armeria/server/Http2RequestDecoder.java 57.14% <100%> (+0.36%) 23 <0> (ø) ⬇️
...ava/com/linecorp/armeria/internal/ChannelUtil.java 91.3% <100%> (+0.82%) 8 <1> (+1) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 751ca56...715b37a. Read the comment docs.

@trustin trustin merged commit 8f6ec10 into line:master Dec 11, 2018
@trustin trustin deleted the higher_connection_window branch December 11, 2018 06:21
@trustin
Copy link
Collaborator Author

trustin commented Dec 11, 2018

Thanks for reviewing.

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

`InboundTrafficController` uses a hard-coded water mark value of 128KiB,
which is much smaller than our current initial connection window size
(1MiB).

Modifications:

- Use the water mark values that matches the initial HTTP/2 connection
  window size.
  - HTTP/1 connections' water mark values stay same.
- Disable Netty `Channel` write buffer water mark to reduce unnecessary
  emission of `channelWritabilityChanged` events.

Result:

- Less frequent network read suspension which was unnecessary.
- Less frequent `channelWritabilityChanged` events.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants