Zero copy between Netty's `Http2Headers` and Armeria's `HttpHeaders` #3817

ikhoon · 2021-09-07T13:44:36Z

Motivation:

Netty's HTTP/2 codec decodes HTTP/2 headers to Http2Headers and
Armeria HTTP/2 codec converts Http2Headers into RequestHeaders,
ResponseHeaders and HttpHeaders.
It is inefficient since it copies headers two times.
After walking through Netty's API, I found out it is possible
to set a custom Http2HeadersDecoder that converts a headers block into
HttpHeaders when building Http2ServerConnectionHandler

Modifications:

Add ArmeriaHttp2Headers that implements Netty's Http2Headers and
internally stores the values in Armeria's HttpHeaders.
Manually create Http2ConnectionEncoder and Http2ConnectionDecoder
on both server and client side to customize Http2HeadersDecoder
Update ArmeriaHttpUtil to directly convert Http2Headers to
HttpHeaders without copies.

Result:

Base work for HTTP/2 headers optimization.

You can now trace HTTP/2 traffic by adjusting the logging level of com.linecorp.armeria.logging.traffic.{server,client}.http2.
For example:

<!-- in your logback.xml -->
<logger name="com.linecorp.armeria.logging.traffic.server.http2" level="TRACE" />
<logger name="com.linecorp.armeria.logging.traffic.client.http2" level="TRACE" />

Motivation: Netty's HTTP/2 codec decodes HTTP/2 headers to `Http2Headers` and Armeria HTTP/2 codec converts `Http2Headers` into `RequestHeaders`, `ResponseHeaders` and `HttpHeaders`. It is inefficient since it copies headers two times. After walking through Netty's API, I found out it is possible to set a custom `Http2HeadersDecoder` coverts a headers block into `HttpHeaders` when building `Http2ServerConnectionHandler` Modifications: - Add `ArmeriaHttp2Headers` that implements Netty's `Http2Headers` and internally stores the values in Armeria's `HttpHeaders`. - Manually create `Http2ConnectionEncoder` and `Http2ConnectionDecoder` on both server and client side to customize `Http2HeadersDecoder` - Update `ArmeriaHttpUtil` to directly convert `Http2Headers` to `HttpHeaders` without copies. Result: TBU

codecov · 2021-09-07T14:57:40Z

Codecov Report

Merging #3817 (7eee550) into master (853e616) will increase coverage by 0.02%.
The diff coverage is 63.10%.

❗ Current head 7eee550 differs from pull request most recent head bd77df5. Consider uploading reports for the commit bd77df5 to get more accurate results

@@             Coverage Diff              @@
##             master    #3817      +/-   ##
============================================
+ Coverage     73.24%   73.26%   +0.02%     
+ Complexity    15082    15052      -30     
============================================
  Files          1326     1318       -8     
  Lines         58003    57745     -258     
  Branches       7356     7330      -26     
============================================
- Hits          42486    42309     -177     
+ Misses        11774    11681      -93     
- Partials       3743     3755      +12

Impacted Files	Coverage Δ
...p/armeria/internal/common/ArmeriaHttp2Headers.java	`50.47% <50.47%> (ø)`
...linecorp/armeria/common/StringMultimapBuilder.java	`56.14% <66.66%> (+16.71%)`	⬆️
...ecorp/armeria/internal/common/ArmeriaHttpUtil.java	`88.23% <93.54%> (-0.09%)`	⬇️
...armeria/client/HttpClientPipelineConfigurator.java	`77.19% <100.00%> (+0.84%)`	⬆️
...java/com/linecorp/armeria/common/HttpResponse.java	`90.47% <100.00%> (-2.79%)`	⬇️
...ia/internal/common/ArmeriaHttp2HeadersDecoder.java	`100.00% <100.00%> (ø)`
...armeria/server/HttpServerPipelineConfigurator.java	`80.82% <100.00%> (+0.90%)`	⬆️
...ecorp/armeria/server/ServerHttp2ObjectEncoder.java	`78.57% <100.00%> (+4.28%)`	⬆️
...p/armeria/server/WrappingTransientHttpService.java	`80.00% <0.00%> (-20.00%)`	⬇️
...rnal/common/GracefulConnectionShutdownHandler.java	`68.08% <0.00%> (-6.39%)`	⬇️
... and 105 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 853e616...bd77df5. Read the comment docs.

trustin

Looks like the good first step for optimizing HTTP/2 headers. Thanks for looking into this, @ikhoon !

minwoox

Left some minor comments.
Nice work!

core/src/main/java/com/linecorp/armeria/common/StringMultimapBuilder.java

core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttp2Headers.java

minwoox · 2021-09-29T01:49:59Z

core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java

-            outputHeaders.add(name, value);
+        final HttpHeadersBuilder builder = inputHeaders.toBuilder();
+
+        for (Entry<AsciiString, AsciiString> disallowed : HTTP_TO_HTTP2_HEADER_DISALLOWED_LIST) {


How about just using a Set for HTTP_TO_HTTP2_HEADER_DISALLOWED_LIST?

Now, I understand that we use this for case-insensitive comparison.
Let's change the name of the map as @trustin suggested. 😉

CaseInsensitiveMap sounds good.

minwoox · 2021-09-29T02:04:52Z

core/src/main/java/com/linecorp/armeria/server/ServerHttp2ObjectEncoder.java

+            builder.remove(HttpHeaderNames.CONTENT_LENGTH);
+        }
+        if (builder == null) {
+            builder = inputHeaders.toBuilder();


How about assigning this builder at the first of the method and remove all if (builder == null)?

Oops... I don't remember why I did like this.🤣
Maybe the code might be changed while fixing test cases.

anuraaga · 2021-09-29T02:37:03Z

Any benchmark results? I remember trying an approach to zero copy a long time ago but found it to reduce performance and abandoned it. The approach was probably different than this one though don't remember - would be good to confirm the performance.

ikhoon · 2021-09-29T03:07:27Z

I ran some benchmarks. Unfortunately, I didn't get any enhancement or regression. 😭
There is still room for performance improvement. I think the approach is not bad and could be a base work for further optimization.

minwoox

Thanks!

ikhoon added the performance label Sep 7, 2021

ikhoon added this to the 1.12.0 milestone Sep 7, 2021

ikhoon requested review from minwoox and trustin as code owners September 7, 2021 13:44

size hint

7eee550

trustin approved these changes Sep 28, 2021

View reviewed changes

minwoox reviewed Sep 29, 2021

View reviewed changes

ikhoon added 2 commits September 29, 2021 12:10

Address comments by @minwoox

6cfeac1

checkstyle

7aa4545

minwoox approved these changes Sep 29, 2021

View reviewed changes

ikhoon and others added 5 commits October 5, 2021 17:00

Merge branch 'master' into zero-copy-headers

8080451

checkstyle

720399e

Rename and checkstyle

0ac4d5d

Update ArmeriaHttpUtilTest.java

368a4d0

Update ArmeriaHttpUtilTest.java

bd77df5

trustin merged commit 755dc71 into line:master Oct 5, 2021

ikhoon deleted the zero-copy-headers branch November 3, 2021 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zero copy between Netty's `Http2Headers` and Armeria's `HttpHeaders` #3817

Zero copy between Netty's `Http2Headers` and Armeria's `HttpHeaders` #3817

ikhoon commented Sep 7, 2021 •

edited

codecov bot commented Sep 7, 2021 •

edited

trustin left a comment

minwoox left a comment

minwoox Sep 29, 2021

minwoox Sep 29, 2021

ikhoon Sep 29, 2021

minwoox Sep 29, 2021

ikhoon Sep 29, 2021

anuraaga commented Sep 29, 2021

ikhoon commented Sep 29, 2021

minwoox left a comment

Zero copy between Netty's Http2Headers and Armeria's HttpHeaders #3817

Zero copy between Netty's Http2Headers and Armeria's HttpHeaders #3817

Conversation

ikhoon commented Sep 7, 2021 • edited

codecov bot commented Sep 7, 2021 • edited

Codecov Report

trustin left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox Sep 29, 2021

Choose a reason for hiding this comment

minwoox Sep 29, 2021

Choose a reason for hiding this comment

ikhoon Sep 29, 2021

Choose a reason for hiding this comment

minwoox Sep 29, 2021

Choose a reason for hiding this comment

ikhoon Sep 29, 2021

Choose a reason for hiding this comment

anuraaga commented Sep 29, 2021

ikhoon commented Sep 29, 2021

minwoox left a comment

Choose a reason for hiding this comment

Zero copy between Netty's `Http2Headers` and Armeria's `HttpHeaders` #3817

Zero copy between Netty's `Http2Headers` and Armeria's `HttpHeaders` #3817

ikhoon commented Sep 7, 2021 •

edited

codecov bot commented Sep 7, 2021 •

edited