Override `HttpRequestDecoder.createMessage()` for perfomance

[echo304]

Motivation:

By overriding HttpRequestDecoder.createMessage(), performance can be improved because there is no need to copy original netty header.

Modifications:

• TBD

Result:

• Closes #4853
• Performance is improved

[echo304]

@jrhee17 This PR is not ready to be reviewed. Sorry about that 🙇 I changed it to draft PR!

[trustin]

Thanks @echo304 for this 🙇 Looking forward to some performance boost!

[trustin]

Gentle ping, @echo304. Looking forward to this improvement! 🤞

[echo304]

I put all the logics about processing headers in Http1RequestDecoder in https://github.com/line/armeria/pull/4863/commits/a2a2f4853f1bb4bfc48b8ff2d1ddcf6ec7b3cd5e (And fixed a bug in https://github.com/line/armeria/pull/4863/commits/deaa03c4b137d8a7f86d9d7a3803673d701dda45)

After that I realized Http1RequestDecoder has too many logic so I moved those ☝️ logics into ArmeriaHttpHeaders.buildRequestHeaders in https://github.com/line/armeria/pull/4863/commits/665c104d17d6738835806b7b07aba60595587123

But it still has some failed test should be fixed 😢

[minwoox]

But it still has some failed test should be fixed

Please, let me know if you find any trouble while fixing it. 😄

[echo304]

With current version, the of com.linecorp.armeria.server.ServerTest#testUnsupportedMethod does fail with below result.

After some investigation, I found that msg of com.linecorp.armeria.server.Http1RequestDecoder#channelRead method is DefaultFullHttpRequest not ArmeriaDefaultHttpRequest.

But I couldn't figure out 1)why the request is mapped with DefaultFullHttpRequest and 2)the location where it's mapped.

@minwoox Can you provide some advice regarding it?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 56.04396% with 80 lines in your changes are missing coverage. Please review.

Project coverage is 74.04%. Comparing base (c6b8906) to head (1eaad3a). Report is 42 commits behind head on main.

Files	Patch %	Lines
...com/linecorp/armeria/server/NettyHttp1Headers.java	31.25%	44 Missing :warning:
...com/linecorp/armeria/server/NettyHttp1Request.java	30.30%	23 Missing :warning:
...ecorp/armeria/internal/common/ArmeriaHttpUtil.java	81.39%	5 Missing and 3 partials :warning:
...a/com/linecorp/armeria/server/HttpServerCodec.java	90.32%	2 Missing and 1 partial :warning:
...corp/armeria/server/ArmeriaHttpHeadersFactory.java	75.00%	1 Missing :warning:
...m/linecorp/armeria/server/Http1RequestDecoder.java	85.71%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4863      +/-   ##
============================================
+ Coverage     74.01%   74.04%   +0.02%     
- Complexity    20799    21068     +269     
============================================
  Files          1803     1825      +22     
  Lines         76617    77775    +1158     
  Branches       9772     9937     +165     
============================================
+ Hits          56708    57587     +879     
- Misses        15289    15513     +224     
- Partials       4620     4675      +55     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

🔍 Build Scan® (commit: 1eaad3ad68d46a84233d2e75f87eef33286e1b8e)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/czs5xvmxoznca
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/oqmj66vxesaf2
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/v6kigesnjvpzq
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/difxzhkj2t45e
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/xl6vhy3zcymge
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/y4di7k733qv5q
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/ixzodof2ogtd2
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/oe5qivz5xs6iu

[jrhee17]

This PR has been stale for a while - I've added a commit to get it moving forward. Changes include:

• Now, only a single header map is created, followed by a "purge" of http1 only headers
  • This design is necessary because netty/armeria assumes the nettyReq still has a CONNECTION header in some places. Hence, the purge is done when the netty request is transformed to an armeria request. Note that from this time, http1-only headers won't be available.
  • The design depends on the fact that there are less HTTP1-only headers than the overall headers (which I think should be the majority of the case anyways)
• Renamed the classes from ArmeriaHttpHeaders, ArmeriaDefaultHttpRequest to NettyHttp1Headers, NettyHttp1Request
  • Also introduced ArmeriaHttpHeadersFactory.java to save more allocation
• Cleaned up some code

Let me know if anything doesn't make sense 🙇

[jrhee17]

I ran com.linecorp.armeria.core.HttpServerBenchmark.plainText a couple times

./gradlew :benchmarks:jmh:jmh \
      -Pjmh.includes='com.linecorp.armeria.core.HttpServerBenchmark.plainText' -Pjmh.params='protocol=H1C' -Pjmh.profilers="async:libPath=$HOME/Projects/async-profiler/build/lib/libasyncProfiler.dylib;output=flamegraph;dir=$HOME/result"

base

Benchmark                            (chunkCount)  (protocol)   Mode  Cnt      Score      Error  Units
HttpServerBenchmark.plainText                 100         H1C  thrpt    5  59030.256 ± 4749.908  ops/s
HttpServerBenchmark.plainText:async           100         H1C  thrpt             NaN               ---

feat

Benchmark                            (chunkCount)  (protocol)   Mode  Cnt      Score      Error  Units
HttpServerBenchmark.plainText                 100         H1C  thrpt    5  60595.652 ± 5476.196  ops/s
HttpServerBenchmark.plainText:async           100         H1C  thrpt             NaN               ---

I don't think there is a significant difference in the simple case. Having said this, Assuming there are more custom headers (which I think better represents the real world) I suppose the performance improvement is more obvious.