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.

Sign up for GitHub

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

Jump to bottom

Http types slow path checks (Fixes #13268) #13269

Merged
merged 4 commits into from
Apr 18, 2023
Merged

Http types slow path checks (Fixes #13268) #13269

merged 4 commits into from
Apr 18, 2023

Conversation

franz1981
Copy link
Contributor

Motivation:

Http encoders/decoders and websocket handlers perform slow type checks
i.e. O(N) search among all implemented interfaces for non-implemented interfaces.

Modifications:

Perform instance exact matches and/or class type check vs common used types, while exposing
unstable APIs to allow users to do the same, if necessary.

Result:

Better performance of Http pipeline with/without websocket handler.
Fixes #13261

@franz1981 franz1981 added improvement benchmark Affect the JMH benchmarks labels Mar 10, 2023
@franz1981
Copy link
Contributor Author

@normanmaurer I'm adding some notes to get feedback

franz1981
franz1981 commented Mar 10, 2023
View reviewed changes
codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java
// fast-path to save expensive O(n) checks; users can override createMessage
// and extends DefaultHttpRequest making implementing HttpResponse:
// this is why we cannot use instanceof DefaultHttpRequest here :(
if (msg.getClass() == DefaultHttpRequest.class) {
Copy link
Contributor Author

@franz1981 franz1981 Mar 10, 2023 •
edited

Choose a reason for hiding this comment

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

@normanmaurer
It's not nice, but it would save the common path (not overridden createMessage) to be super fast. without hitting the slow path (not implemented HttpResponse type check)

@franz1981 franz1981 marked this pull request as draft March 10, 2023 08:56
@franz1981
Copy link
Contributor Author

The changes on WebSocketServerExtensionHandler enabling user code to optimize their own check require some attention, IMO

@franz1981
Copy link
Contributor Author

franz1981 commented Mar 10, 2023 •
edited

In term of performance improvement, the micros reports...
Without 6e1558f2390f95aebad5dbc17ca67dd5140ce722:

Benchmark                                  (websocket)   Mode  Cnt       Score       Error  Units
HttpRequestResponseBenchmark.netty                true  thrpt   20  747540.482 ±  5546.911  ops/s
HttpRequestResponseBenchmark.netty               false  thrpt   20  830114.142 ± 18724.768  ops/s

With 6e1558f2390f95aebad5dbc17ca67dd5140ce722:

Benchmark                                  (websocket)   Mode  Cnt       Score      Error  Units
HttpRequestResponseBenchmark.netty                true  thrpt   20  854429.400 ± 4088.882  ops/s
HttpRequestResponseBenchmark.netty               false  thrpt   20  888279.220 ± 6643.213  ops/s

that shows an improvement both with/without websocket handlers, as stated in the commit description.

@franz1981 franz1981 marked this pull request as ready for review March 10, 2023 09:46
@franz1981
Http types slow path checks (Fixes netty#13268)
d88e63a 
Motivation:

Http encoders/decoders and websocket handlers perform slow type checks
i.e. O(N) search among all implemented interfaces for non-implemented interfaces.

Modifications:

Perform instance exact matches and/or class type check vs common used types, while exposing
unstable APIs to allow users to do the same, if necessary.

Result:

Better performance of Http pipeline with/without websocket handler.
Fixes netty#13261
@franz1981
Copy link
Contributor Author

Done @chrisvest let me know if the code paths I've commented myself seems ok to you

normanmaurer
normanmaurer approved these changes Mar 17, 2023
View reviewed changes
Copy link
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

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

LGTM... just 3 nits

microbench/src/main/java/io/netty/microbench/http/HttpRequestResponseBenchmark.java Show resolved Hide resolved
microbench/src/main/java/io/netty/microbench/http/HttpRequestResponseBenchmark.java Show resolved Hide resolved
codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseEncoder.java Outdated Show resolved Hide resolved
@franz1981
Copy link
Contributor Author

@normanmaurer @trustin added the comments as requested:sorry for the delay and the bad coding :"(
Sadly, differently from https://bugs.openjdk.org/browse/JDK-8180450 this perf issue won't be solved on OpenJDK (probably Graal uses a different implementation for type checks vs non implemented interfaces) and it affects performance (as benchs shows).

@franz1981
Copy link
Contributor Author

@chrisvest addressed all the comments: I'm still quite unhappy about the changes, but I have no other solution to the slow path problem

@chrisvest chrisvest merged commit f67a556 into netty:4.1 Apr 18, 2023
14 checks passed
@chrisvest
Copy link
Contributor

@franz1981 Thanks!

Do you have plans for making similar PRs for Netty 5? I assume the type profiles will have to be looked at again to see what checks are potential hot spots.

@chrisvest chrisvest added this to the 4.1.92.Final milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisvest chrisvest chrisvest approved these changes

@normanmaurer normanmaurer normanmaurer approved these changes

@trustin trustin Awaiting requested review from trustin

Assignees
No one assigned
Labels
benchmark Affect the JMH benchmarks improvement
Projects
None yet
Milestone
4.1.92.Final
Development

Successfully merging this pull request may close these issues.

HTTP/2 ALPN not working with BC FIPS TLS provider
3 participants
@franz1981 @chrisvest @normanmaurer