HttpObjectEncoder scalability issue due to instanceof checks (Fixes #12708) #12709
Conversation
franz1981
added
defect
improvement
benchmark
franz1981
force-pushed
the
netty-4.1.74.Final
branch
from
69a3fd4
to
34eff3c
Compare
|
@franz1981 can you share any numbers that "proof" the gain ? |
|
Yep @normanmaurer I'm going publish both an end 2 end benchmark result and the micro-bench one I've modified in Netty, but I'm trying a different fix too, that won't requires holding any |
|
@normanmaurer
The machine is a 32 cores one, if we run the same tests with a single core, there's no slowdown (as expected, given that's a contention issue) - not really, because the "type pollution" still cause instanceof to happen for real, making it slower, but without scalability impacts. |
|
I am trying hard to create a version of this fix that doesn't require any hint re the "full concrete http type" used, making it more transparent for existing users so, please, hold on :) |
|
This is the result of the microbenchmark while using type pollution This PR 1 thread: This PR 6 threads: 4.1 1 thread: 4.1 6 threads Few notes:
There's still some work to do for HTTP but this one already improve things dramatically, see
|
franz1981
force-pushed
the
netty-4.1.74.Final
branch
from
34eff3c
to
b108ec2
Compare
| return ((FileRegion) msg).count(); | ||
| } | ||
| throw new IllegalStateException("unexpected message type: " + StringUtil.simpleClassName(msg)); | ||
| return msg instanceof FullHttpMessage || |
There was a problem hiding this comment.
@normanmaurer
We can add checks for existing known concrete types too (default, default full etc etc), but TBH users can do it too while extending the resp/req encoders based on how they use it ie if they have custom types or not.
note:
The sequence of checks here matches the same on encode in order to let concrete types to not get their Klass cache field invalidated (and refreshed) - this will likely cause a perf hit for the last ones checked in the chain (ByteBuf and FileRegion), because they will be evaluated after every previous checks always hitting the JIT O(n) slow path.
|
@chrisvest ready to go :) |
franz1981
force-pushed
the
netty-4.1.74.Final
branch
3 times, most recently
from
8381793
to
88e2c30
Compare
|
weird failure on @normanmaurer @chrisvest how to re-run the validations/just that validations? |
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
Show resolved
Hide resolved
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
Outdated
Show resolved
Hide resolved
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
Show resolved
Hide resolved
Agree, and it's unfortunate that such JDK 'feature" is unlikely to be fixed but still - there's a lesson to learn here too ie using traits to guide a state machine isn't a good idea, because it relies on how good is the runtime to resolve it. With polymorphism is similar, although maybe a double dispatch/visitor pattern would save the klass caching mechanism to be triggered. Probably a plain old int/byte field in a single common type acting as a bitset of known "traits" + some switches to choose what to do for each of the implemented traits, is more deterministic regardless what the JIT decide (meaning that its performance characteristics survive over time/JDK bugs). |
franz1981
force-pushed
the
netty-4.1.74.Final
branch
2 times, most recently
from
97e1c1e
to
200cbd2
Compare
|
@normanmaurer this is ready to go, unless there are other concerns - I can add more comments to help |
Let me add some comments on the PR |
…etty#12708) Motivation: Current http encoder logic cause contention over the klass field used by the JIT to speed up instanceof checks vs interfaces, preventing scaling with multi-core machines. See https://bugs.openjdk.org/browse/JDK-8180450 for more info. Modifications: Code duplication to reduce the arity of the morphism on the instanceof checks on the http encoder. Removed using encoder inherited methods to take control of the arity of the call-site morphism while releasing ref counted http msg types. Result: Scalable HTTP encoding
franz1981
force-pushed
the
netty-4.1.74.Final
branch
from
200cbd2
to
5244ca7
Compare
|
@normanmaurer comment added and the issue too is more verbose (with an example that show what's going on) |
|
The benchmark I've made is naive, but is good enough for our purpose I believe - type pollution made upfront just assume LOT of things re JIT and its configuration (and which optimizations should perform) |
|
@franz1981 thanks a lot for all the hard work. I learned something new ❤️ |
|
@franz1981 can you also do a PR for main ? |
|
@normanmaurer doing it for main too ;) Let's chat on the pr |
Agree... but better to be consistent until we d have a better way in main |
…tly (Fixes netty#12750) Motivation: Changes due to netty#12709 have added a code path. Modifications: Restore the "just http message" case as separated by the http content ones Result: Same encoding as original code
Motivation: netty#12709 changed HttpObjectEncoder to override the write method of MessageToMessageEncoder, with slightly changed semantics: The `msg` argument to `encode` is not released anymore. To accommodate this change, netty#12709 also update `HttpObjectEncoder.encode` to release the `msg`. However, `HttpClientCodec.Encoder` overrides `encode` and simply forwards the message if a HTTP upgrade has been completed. This code path was not updated to release the input message. This leads to a memory leak. Modifications: Changed the `encode` implementation to not retain the message that is forwarded. Added a test case to verify that the refCnt to the data passed through is unchanged. Result: The buffer retains its correct refCnt and will be released properly.
Motivation: #12709 changed HttpObjectEncoder to override the write method of MessageToMessageEncoder, with slightly changed semantics: The `msg` argument to `encode` is not released anymore. To accommodate this change, #12709 also update `HttpObjectEncoder.encode` to release the `msg`. However, `HttpClientCodec.Encoder` overrides `encode` and simply forwards the message if a HTTP upgrade has been completed. This code path was not updated to release the input message. This leads to a memory leak. Modifications: Changed the `encode` implementation to not retain the message that is forwarded. Added a test case to verify that the refCnt to the data passed through is unchanged. Result: The buffer retains its correct refCnt and will be released properly.
Motivation:
Current http encoder logic cause contention over the klass field used by the JIT to speed up instanceof checks vs interfaces,
preventing scaling with multi-core machines.
See https://bugs.openjdk.org/browse/JDK-8180450 for more info.
Modifications:
Code duplication to reduce the arity of the morphism on the instanceof checks on the http encoder.
Removed using encoder inherited methods to take control of the arity of the
call-site morphism while releasing ref counted http msg types.
Result:
Scalable HTTP encoding