Override `HttpRequestDecoder.createMessage()` for perfomance #4863

echo304 · 2023-05-06T14:16:35Z

Motivation:

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

Modifications:

TBD

Result:

Closes Override HttpRequestDecoder.createMessage() for perfomance #4853
Performance is improved

jrhee17

Thanks! Left some preliminary comments

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

echo304 · 2023-05-08T03:06:51Z

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

trustin · 2023-05-08T10:39:04Z

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

echo304 · 2023-05-09T14:04:24Z

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

+
+        @Override
+        protected HttpMessage createMessage(String[] initialLine) throws Exception {
+            return new DefaultHttpRequest(


After some investigation, I found the logic that converts Netty request to Armeria headers.

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

Lines 627 to 649 in 92e7017

public static RequestHeaders toArmeria(

ChannelHandlerContext ctx, HttpRequest in,

ServerConfig cfg, String scheme, RequestTarget reqTarget) throws URISyntaxException {

final io.netty.handler.codec.http.HttpHeaders inHeaders = in.headers();

final RequestHeadersBuilder out = RequestHeaders.builder();

out.sizeHint(inHeaders.size());

out.method(firstNonNull(HttpMethod.tryParse(in.method().name()), HttpMethod.UNKNOWN))

.scheme(scheme)

.path(reqTarget.toString());

// Add the HTTP headers which have not been consumed above

toArmeria(inHeaders, out);

if (!out.contains(HttpHeaderNames.HOST)) {

// The client violates the spec that the request headers must contain a Host header.

// But we just add Host header to allow the request.

// https://datatracker.ietf.org/doc/html/rfc7230#section-5.4

final String defaultHostname = cfg.defaultVirtualHost().defaultHostname();

final int port = ((InetSocketAddress) ctx.channel().localAddress()).getPort();

out.add(HttpHeaderNames.HOST, defaultHostname + ':' + port);

}

return out.build();

}

At the very beginning, I thought that we can implements io.netty.handler.codec.http.HttpRequest and create our own HttpRequest implementation to replace DefaultHttpRequest.
But with this design, we still need ArmeriaHttpUtil.toArmeria some part of the logic because this class(HttpServerCodec) depends on netty's interface.

Do you have any advice on it? @minwoox @ikhoon

I thought that we can implements io.netty.handler.codec.http.HttpRequest and create our own HttpRequest implementation to replace DefaultHttpRequest.

I think it's the way to go. 👍 We can probably create RequestHeadersBuilder in it and passes it in Http1RequestDecoder.

we still need ArmeriaHttpUtil.toArmeria some part of the logic because this class(HttpServerCodec) depends on netty's interface.

Yeah, we can apply the logic at once in the Http1RequestDecoder or we can just use the logic inside of the HttpHeaders that is returned by HttpMessage.headers().

Does this make sense? 🤔

Just want to double check. ✅

Is it what we want to improve/replace?

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

Lines 173 to 175 in 361c115

// Convert the Netty HttpHeaders into Armeria RequestHeaders.

final RequestHeaders headers =

ArmeriaHttpUtil.toArmeria(ctx, nettyReq, cfg, scheme.toString(), reqTarget);

It's a little bit difficult to understand task than I expected so I wanted to make sure I'm on the right direction. 😏

Yes, exactly. 👍
We can define our own DefaultHttpRequest and return it in createMessage:

@Override protected HttpMessage createMessage(String[] initialLine) throws Exception { return new ArmeriaDefaultHttpRequest(HttpMethod.valueOf(initialLine[0]), initialLine[1]); } static final class ArmeriaDefaultHttpRequest implements HttpRequest { private final HttpMethod method; private final RequestHeadersBuilder builder; private final HttpHeaders headers; ArmeriaDefaultHttpRequest(HttpMethod method, String uri) { this.method = method; builder = RequestHeaders.builder( com.linecorp.armeria.common.HttpMethod.valueOf(method.name()), uri); headers = new ArmeriaHttpHeaders(builder); } @Override public HttpMethod method() { return method; } @Override public HttpHeaders headers() { return headers; } ... }

We also need ArmeriaHttpHeaders class and it will prevents adding not allowed headers:

final class ArmeriaHttpHeaders extends HttpHeaders { private final RequestHeadersBuilder builder; ArmeriaHttpHeaders(RequestHeadersBuilder builder) { this.builder = builder; } @Override public String get(String name) { return builder.get(name); } @Override public Integer getInt(CharSequence name) { return builder.getInt(name); } @Override public HttpHeaders add(String name, Object value) { // We can add a logic that prevents adding invalid headers that is defined in ArmeriaHttpUtil.toArmeria() return builder.add(name, value); } ... }

In Http1RequestDecoder, we will bring the RequestHeadersBuilder from the Netty request and add a host header if needed:

if (nettyReq instanceof ArmeriaDefaultHttpRequest) { final RequestHeadersBuilder builder = ((ArmeriaDefaultHttpRequest) nettyReq).requestHeadersBuilder(); }

This is what I have imagined. 😉
Please let me know if it works or not.

Now I started to understand the way we have to go. 😏
Let me clarify what you're saying:

with ArmeriaHttpHeaders which extends io.netty.handler.codec.http.HttpHeaders,
we can store actual data inside HttpHeaders with RequestHeadersBuilder builder.

And we can build com.linecorp.armeria.common.RequestHeaders inside Http1RequestDecoder with ((ArmeriaDefaultHttpRequest) nettyReq).requestHeadersBuilder();

And hopefully, RequestHeadersBuilder builder already contains most of the data that we need, many redundant process inside toArmeria can be removed. (especially, here 👇 )

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

Lines 681 to 721 in 323fb45

public static void toArmeria(io.netty.handler.codec.http.HttpHeaders inHeaders, HttpHeadersBuilder out) {

final Iterator<Entry<CharSequence, CharSequence>> iter = inHeaders.iteratorCharSequence();

// Choose 8 as a default size because it is unlikely we will see more than 4 Connection headers values,

// but still allowing for "enough" space in the map to reduce the chance of hash code collision.

final CaseInsensitiveMap connectionDisallowedList =

toLowercaseMap(inHeaders.valueCharSequenceIterator(HttpHeaderNames.CONNECTION), 8);

StringJoiner cookieJoiner = null;

while (iter.hasNext()) {

final Entry<CharSequence, CharSequence> entry = iter.next();

final AsciiString aName = HttpHeaderNames.of(entry.getKey()).toLowerCase();

if (HTTP_TO_HTTP2_HEADER_DISALLOWED_LIST.contains(aName) ||

connectionDisallowedList.contains(aName)) {

final CharSequence value = entry.getValue();

if (!maybeWebSocketUpgrade(aName, value)) {

continue;

}

}

// https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2 makes a special exception for TE

if (aName.equals(HttpHeaderNames.TE)) {

toHttp2HeadersFilterTE(entry, out);

continue;

}

// Cookies must be concatenated into a single octet string.

// https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.5

final CharSequence value = entry.getValue();

if (aName.equals(HttpHeaderNames.COOKIE)) {

if (cookieJoiner == null) {

cookieJoiner = new StringJoiner(COOKIE_SEPARATOR);

}

COOKIE_SPLITTER.split(value).forEach(cookieJoiner::add);

} else {

out.add(aName, convertHeaderValue(aName, value));

}

}

if (cookieJoiner != null && cookieJoiner.length() != 0) {

out.add(HttpHeaderNames.COOKIE, cookieJoiner.toString());

}

}

Is it correct?

But my concern is that implementing own ArmeriaHttpHeaders is pretty huge task. (Well, obviously this is unavoidable)

Is it correct?

That's correct. 😄

But my concern is that implementing own ArmeriaHttpHeaders is pretty huge task. (Well, obviously this is unavoidable)

Yes, it is. But we don't have to implement all of the methods. It's only used in the pipeline so we can implement the methods that are called in the handler. 😉

🙆‍♂️
Let me show you some code as soon as possible even if it's not completed!

- Custom Headers for Armeria which extends `HttpHeaders` - It delegates most of the header value storing to given `RequestHeadersBuilder`

- Custom `HttpRequest` that replaces `DefaultHttpRequest` from `HttpServerCodec`

trustin · 2023-05-30T07:47:01Z

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

…est`

…quest`

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

minwoox

Looks good overall. 😉

minwoox · 2023-06-02T13:21:37Z

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

-                    final RequestHeaders headers =
-                            ArmeriaHttpUtil.toArmeria(ctx, nettyReq, cfg, scheme.toString(), reqTarget);
+                    final RequestHeaders headers;
+                    if (nettyReq instanceof ArmeriaDefaultHttpRequest) {


We could probably do
assert nettyReq instanceof ArmeriaDefaultHttpRequest

minwoox · 2023-06-02T13:22:02Z

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

+
+                        final ArmeriaDefaultHttpRequest armeriaDefaultHttpRequest = (ArmeriaDefaultHttpRequest) nettyReq;
+                        final HttpHeaders inHeaders = armeriaDefaultHttpRequest.headers();
+
+                        builder.sizeHint(inHeaders.size());


We don't need to set the sizeHint:

Suggested change

final ArmeriaDefaultHttpRequest armeriaDefaultHttpRequest = (ArmeriaDefaultHttpRequest) nettyReq;

final HttpHeaders inHeaders = armeriaDefaultHttpRequest.headers();

builder.sizeHint(inHeaders.size());

minwoox · 2023-06-02T13:24:01Z

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

+import io.netty.handler.codec.http.HttpStatusClass;
+import io.netty.handler.codec.http.HttpVersion;
+
+public final class HttpServerCodec extends CombinedChannelDuplexHandler<HttpRequestDecoder, HttpResponseEncoder>


Could you use this class instead of Netty's one from here?

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

Line 127 in b78d951

ctx.pipeline().remove(HttpServerCodec.class);

minwoox · 2023-06-02T13:24:22Z

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

@@ -0,0 +1,199 @@
+package com.linecorp.armeria.internal.common;


missing copyright

minwoox · 2023-06-02T13:25:38Z

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

+import io.netty.handler.codec.http.HttpHeaders;
+import io.netty.util.AsciiString;
+
+public final class ArmeriaHttpHeaders extends HttpHeaders {


We can put this method in the package com.linecorp.armeria.server and remove the public modifier.

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

minwoox · 2023-06-02T13:34:17Z

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

+import io.netty.handler.codec.http.HttpVersion;
+import io.netty.util.internal.ObjectUtil;
+
+public class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {


Suggested change

public class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {

final class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {

We can remove the public modifiers in this class.

minwoox · 2023-06-02T13:35:09Z

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

+     * @param method      the HTTP method of the request
+     * @param uri         the URI or path of the request
+     */
+    public ArmeriaDefaultHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri) {


Let's remove the constructor which isn't used.

minwoox · 2023-06-02T13:35:14Z

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

+     * @param uri               the URI or path of the request
+     * @param givenHeaders      the Headers for this Request
+     */
+    public ArmeriaDefaultHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri, HttpHeaders givenHeaders) {


minwoox · 2023-06-02T13:36:07Z

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

+    }
+
+//    @Override
+//    public String toString() {


We can use MoreObjects.toStringHelper() for toString().

…tself

echo304 · 2023-06-08T11:59:37Z

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

+            splitByCommaAndAdd(connectionDisallowedList, lowerCased);
+        }
+
+        if (HTTP_TO_HTTP2_HEADER_DISALLOWED_LIST.contains(asciiName) ||


With this line of code, this headers won't contain Transfer-Encoding header.
As a result, below test failed because it considers corresponding request as if endOfStream.

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

Line 262 in b808c3e

final boolean endOfStream = contentEmpty && !HttpUtil.isTransferEncodingChunked(nettyReq);

So I'm trying to solve these kind of regression without copying another header.

Oops, I missed this comment. If so, I think we can remove the Transfer-Encoding header in Http1RequestDecoder after isTransferEncodingChunked is executed. WDYT?

@minwoox Yeah, you're right.
I tried another approach that moved isTransferEncodingChunked up before filtering out disallowed headers.
a2a2f48#diff-a3306f0289f199b1657bb0e32849fd97502c82a91318adc373f8e750a96f84ecR187-R188

minwoox · 2023-07-06T02:06:42Z

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

+        builder = RequestHeaders.builder(
+                com.linecorp.armeria.common.HttpMethod.valueOf(method.name()), uri);


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?

It seems like an exception is raised while converting the method. We need to fix it:

Suggested change

builder = RequestHeaders.builder(

com.linecorp.armeria.common.HttpMethod.valueOf(method.name()), uri);

final com.linecorp.armeria.common.HttpMethod armeriaMethod =

com.linecorp.armeria.common.HttpMethod.tryParse(method.name());

builder = RequestHeaders.builder(armeriaMethod != null ? armeriaMethod

: com.linecorp.armeria.common.HttpMethod.UNKNOWN,

uri);

ikhoon · 2023-07-14T02:34:48Z

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

+import io.netty.handler.codec.http.HttpVersion;
+import io.netty.util.internal.ObjectUtil;
+
+final class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {


Is there a specific reason to extend DefaultHttpMessage? That internally creates DefaultHttpHeaders again. Duplicate headers may result in performance degression.

Suggested change

final class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {

final class ArmeriaHttp1Request implements HttpRequest {

ikhoon · 2023-07-14T02:48:50Z

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

+     */
+    public ArmeriaHttpHeaders(RequestHeadersBuilder builder, HttpHeaders headers) {
+        this.builder = builder;
+        headers.forEach(e -> add(e.getKey(), e.getValue()));


This copies the Netty headers to Armeria Headers. Unfortunately, it does not match with the goal of this PR.

there is no need to copy original netty header.

What do you think of lazily creating or mutating fields when necessary?

private final HttpHeaders nettyHeaders; @Nullable private RequestHeaders armeriaHeaders; public ArmeriaHttpHeaders(RequestHeadersBuilder builder, HttpHeaders headers) { this.headers = headers; } @Override public String get(String name) { if (armeriaHeaders != null) { return armeriaHeaders.get(name); } if (needsValidation(name)) { // Create Armeria headers and return; } // Use Netty Headers as is. return headers.get(name); } public List<Entry<String, String>> entries() { // Create Armeria Headers if null } public Iterator<Entry<String, String>> iterator() { // Create Armeria Headers if null }

Thank you for pointing out!
Let me take a look into it :)

I've reviewed your idea and bring some thoughts.

Firstly, I totally agree that current approach doesn't resolve the problem of copying headers 🤦

And specifically against your advice, Http1RequestDecoder has plenty of logic that utilize RequestHeaders. So I think we need to convert(create) HttpHeaders to RequestHeaders most of the time even with 'lazy initializing' strategy. Which means it goes back to copying headers situation again.
I'm not sure if 'lazy initialization' strategy could provide significant value. 🤔 Correct me if I'm wrong.

With above idea, I'm afraid we need to copy headers somewhere in our system anyway.

This copies the Netty headers to Armeria Headers. Unfortunately, it does not match with the goal of this PR.

there is no need to copy original netty header.

What do you think of lazily creating or mutating fields when necessary?

private final HttpHeaders nettyHeaders; @Nullable private RequestHeaders armeriaHeaders; public ArmeriaHttpHeaders(RequestHeadersBuilder builder, HttpHeaders headers) { this.headers = headers; } @Override public String get(String name) { if (armeriaHeaders != null) { return armeriaHeaders.get(name); } if (needsValidation(name)) { // Create Armeria headers and return; } // Use Netty Headers as is. return headers.get(name); } public List<Entry<String, String>> entries() { // Create Armeria Headers if null } public Iterator<Entry<String, String>> iterator() { // Create Armeria Headers if null }

codecov · 2024-03-25T08:06:43Z

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 ⚠️
...com/linecorp/armeria/server/NettyHttp1Request.java	30.30%	23 Missing ⚠️
...ecorp/armeria/internal/common/ArmeriaHttpUtil.java	81.39%	5 Missing and 3 partials ⚠️
...a/com/linecorp/armeria/server/HttpServerCodec.java	90.32%	2 Missing and 1 partial ⚠️
...corp/armeria/server/ArmeriaHttpHeadersFactory.java	75.00%	1 Missing ⚠️
...m/linecorp/armeria/server/Http1RequestDecoder.java	85.71%	0 Missing and 1 partial ⚠️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-03-25T08:52:36Z

🔍 Build Scan® (commit: `1eaad3a`)

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 · 2024-03-26T11:36:44Z

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 · 2024-03-26T12:04:08Z

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

@@ -696,6 +693,8 @@ public static HttpHeaders toArmeria(io.netty.handler.codec.http.HttpHeaders inHe

    /**
     * Converts the specified Netty HTTP/1 headers into Armeria HTTP/2 headers.
+     * Functionally, this method is expected to behavior in the same way as
+     * {@link #purgeHttp1OnlyHeaders(io.netty.handler.codec.http.HttpHeaders, HttpHeadersBuilder)}.
     */


Note that I haven't migrated this method to use purgeHttp1OnlyHeaders since this is probably slower for other code paths using this method without zero-copy

jrhee17

I believe this PR is ready for review by other maintainers - changes look good to me for merging

minwoox

I've also put a patch that

hides static methods in ArmeriaHttpUtil
changes ArmeriaHttpHeadersFactory to enum

Thanks!

ikhoon

Thanks, @echo304!
I'm looking forward to seeing benchmark results. What do you think of sending a PR to https://github.com/TechEmpower/FrameworkBenchmarks/blob/master/frameworks/Java/armeria/pom.xml#L19 when a new version is released?

…ine#4863)" This reverts commit 8a99f23.

jrhee17 · 2024-04-29T05:48:34Z

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.

Fork netty's httpServerCodec and replace its usage

1de569a

echo304 requested review from ikhoon, jrhee17, minwoox and trustin as code owners May 6, 2023 14:16

Fix test compile failure

6b41e20

jrhee17 reviewed May 8, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/HttpServerCodec.java Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/server/HttpServerCodec.java Show resolved Hide resolved

jrhee17 added the performance label May 8, 2023

jrhee17 added this to the 1.24.0 milestone May 8, 2023

echo304 marked this pull request as draft May 8, 2023 03:05

Add licenses

ffa914f

echo304 commented May 9, 2023

View reviewed changes

echo304 added 2 commits May 29, 2023 20:52

Implement ArmeriaHttpHeaders

9911367

- Custom Headers for Armeria which extends `HttpHeaders` - It delegates most of the header value storing to given `RequestHeadersBuilder`

Implement ArmeriaDefaultHttpRequest

b688903

- Custom `HttpRequest` that replaces `DefaultHttpRequest` from `HttpServerCodec`

echo304 added 2 commits May 30, 2023 17:02

Modify HttpServerCodec.createMessage to use `ArmeriaDefaultHttpRequ…

9de5522

…est`

Modify Http1RequestDecoder.channelRead to use `ArmeriaDefaultHttpRe…

743a6fc

…quest`

echo304 marked this pull request as ready for review June 2, 2023 12:23

Merge branch 'main' into custom-http-server-codec

efd741e

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

minwoox reviewed Jun 2, 2023

View reviewed changes

echo304 added 5 commits June 3, 2023 21:39

Reflect code review

bba7d8d

Fix styles

8bccbc8

Fix constructor of ArmeriaHttpHeaders to invoke add() method of i…

65ad9cd

…tself

Add headers() getter on ArmeriaDefaultHttpRequest

ce07dc1

Add test for ArmeriaHttpHeaders and fix logic

b81f037

ikhoon modified the milestones: 1.24.0, 1.25.0 Jun 6, 2023

echo304 commented Jun 8, 2023

View reviewed changes

echo304 added 3 commits July 5, 2023 22:26

Fix ArmeriaHttpHeadersTest failed tests

7515d8f

Merge branch 'main' into custom-http-server-codec

89bc4ea

Fix failed shouldSupportBindingOnDomainSocket test

39092ea

minwoox reviewed Jul 6, 2023

View reviewed changes

echo304 added 3 commits July 6, 2023 17:22

Fix failed testUnsupportedMethod test

c9f7520

Fix styles

9ecd024

Merge branch 'main' into custom-http-server-codec

2cc3f13

ikhoon requested changes Jul 14, 2023

View reviewed changes

jrhee17 modified the milestones: 1.25.0, 1.26.0 Jul 18, 2023

minwoox modified the milestones: 1.26.0, 1.27.0 Oct 11, 2023

ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 5, 2024

Merge branch 'main' into custom-http-server-codec

f48f148

minor cleanups

19ab386

jrhee17 reviewed Mar 26, 2024

View reviewed changes

jrhee17 added 2 commits March 26, 2024 21:05

hide over-exposed class

340a6f5

lint

ca51df6

jrhee17 approved these changes Mar 28, 2024

View reviewed changes

Hide static methods and Change ArmeriaHttpHeadersFactory to enum

c80eb0c

minwoox approved these changes Apr 2, 2024

View reviewed changes

minor fix

1eaad3a

ikhoon approved these changes Apr 9, 2024

View reviewed changes

jrhee17 merged commit 8a99f23 into line:main Apr 11, 2024
17 of 18 checks passed

jrhee17 added a commit to jrhee17/armeria that referenced this pull request Apr 29, 2024

Revert "Override HttpRequestDecoder.createMessage() for perfomance (l…

44f4bdc

…ine#4863)" This reverts commit 8a99f23.

	public static RequestHeaders toArmeria(
	ChannelHandlerContext ctx, HttpRequest in,
	ServerConfig cfg, String scheme, RequestTarget reqTarget) throws URISyntaxException {

	final io.netty.handler.codec.http.HttpHeaders inHeaders = in.headers();
	final RequestHeadersBuilder out = RequestHeaders.builder();
	out.sizeHint(inHeaders.size());
	out.method(firstNonNull(HttpMethod.tryParse(in.method().name()), HttpMethod.UNKNOWN))
	.scheme(scheme)
	.path(reqTarget.toString());

	// Add the HTTP headers which have not been consumed above
	toArmeria(inHeaders, out);
	if (!out.contains(HttpHeaderNames.HOST)) {
	// The client violates the spec that the request headers must contain a Host header.
	// But we just add Host header to allow the request.
	// https://datatracker.ietf.org/doc/html/rfc7230#section-5.4
	final String defaultHostname = cfg.defaultVirtualHost().defaultHostname();
	final int port = ((InetSocketAddress) ctx.channel().localAddress()).getPort();
	out.add(HttpHeaderNames.HOST, defaultHostname + ':' + port);
	}
	return out.build();
	}

	// Convert the Netty HttpHeaders into Armeria RequestHeaders.
	final RequestHeaders headers =
	ArmeriaHttpUtil.toArmeria(ctx, nettyReq, cfg, scheme.toString(), reqTarget);

	public static void toArmeria(io.netty.handler.codec.http.HttpHeaders inHeaders, HttpHeadersBuilder out) {
	final Iterator<Entry<CharSequence, CharSequence>> iter = inHeaders.iteratorCharSequence();
	// Choose 8 as a default size because it is unlikely we will see more than 4 Connection headers values,
	// but still allowing for "enough" space in the map to reduce the chance of hash code collision.
	final CaseInsensitiveMap connectionDisallowedList =
	toLowercaseMap(inHeaders.valueCharSequenceIterator(HttpHeaderNames.CONNECTION), 8);
	StringJoiner cookieJoiner = null;
	while (iter.hasNext()) {
	final Entry<CharSequence, CharSequence> entry = iter.next();
	final AsciiString aName = HttpHeaderNames.of(entry.getKey()).toLowerCase();
	if (HTTP_TO_HTTP2_HEADER_DISALLOWED_LIST.contains(aName) \|\|
	connectionDisallowedList.contains(aName)) {
	final CharSequence value = entry.getValue();
	if (!maybeWebSocketUpgrade(aName, value)) {
	continue;
	}
	}

	// https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.2 makes a special exception for TE
	if (aName.equals(HttpHeaderNames.TE)) {
	toHttp2HeadersFilterTE(entry, out);
	continue;
	}

	// Cookies must be concatenated into a single octet string.
	// https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.5
	final CharSequence value = entry.getValue();
	if (aName.equals(HttpHeaderNames.COOKIE)) {
	if (cookieJoiner == null) {
	cookieJoiner = new StringJoiner(COOKIE_SEPARATOR);
	}
	COOKIE_SPLITTER.split(value).forEach(cookieJoiner::add);
	} else {
	out.add(aName, convertHeaderValue(aName, value));
	}
	}

	if (cookieJoiner != null && cookieJoiner.length() != 0) {
	out.add(HttpHeaderNames.COOKIE, cookieJoiner.toString());
	}
	}

		@@ -0,0 +1,199 @@
		package com.linecorp.armeria.internal.common;

	public class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {
	final class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {

		builder = RequestHeaders.builder(
		com.linecorp.armeria.common.HttpMethod.valueOf(method.name()), uri);

-        builder = RequestHeaders.builder(
-                com.linecorp.armeria.common.HttpMethod.valueOf(method.name()), uri);
+        final com.linecorp.armeria.common.HttpMethod armeriaMethod =
+                com.linecorp.armeria.common.HttpMethod.tryParse(method.name());
+        builder = RequestHeaders.builder(armeriaMethod != null ? armeriaMethod
+                                                               : com.linecorp.armeria.common.HttpMethod.UNKNOWN,
+                                         uri);

	final class ArmeriaDefaultHttpRequest extends DefaultHttpMessage implements HttpRequest {
	final class ArmeriaHttp1Request implements HttpRequest {

Override HttpRequestDecoder.createMessage() for perfomance #4863

Override HttpRequestDecoder.createMessage() for perfomance #4863

Conversation

echo304 commented May 6, 2023

jrhee17 left a comment

Choose a reason for hiding this comment

echo304 commented May 8, 2023

trustin commented May 8, 2023

echo304 May 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echo304 May 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echo304 May 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin commented May 30, 2023

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echo304 Jun 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 25, 2024 • edited

Codecov Report

github-actions bot commented Mar 25, 2024 • edited

🔍 Build Scan® (commit: 1eaad3a)

jrhee17 commented Mar 26, 2024

Choose a reason for hiding this comment

jrhee17 left a comment • edited

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

jrhee17 commented Apr 29, 2024 • edited

base

feat

Override `HttpRequestDecoder.createMessage()` for perfomance #4863

Override `HttpRequestDecoder.createMessage()` for perfomance #4863

echo304 May 9, 2023 •

edited

echo304 May 23, 2023 •

edited

echo304 May 26, 2023 •

edited

echo304 Jun 30, 2023 •

edited

codecov bot commented Mar 25, 2024 •

edited

github-actions bot commented Mar 25, 2024 •

edited

🔍 Build Scan® (commit: `1eaad3a`)

jrhee17 left a comment •

edited

jrhee17 commented Apr 29, 2024 •

edited