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.

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

Redo fix scalability issue due to checkcast on context's invoke operations #13741

Merged

Conversation

franz1981
Copy link
Contributor

@franz1981 franz1981 commented Dec 19, 2023

Motivation:

ChannelDuplexHandler can implements both ChannelOutboundHandler and ChannelInboundHandler causing a scalability issue due to checkcast due to https://bugs.openjdk.org/browse/JDK-8180450
Not only: there are different classes eg Http2ConnectionHandler, which implement them transitively, by using one of the 2 existing adapters (ChannelInboundAdapter, ChanneOutboundAdapters). The existing change at #12806 was fixing only the duplex cases, but others like the above one was still affected.

Modifications:

Replace the duplex type checks with broader inbound adapter ones, given that duplex is still based on it. Add outbound adapters type checks in addition to duplex ones.

Result:

More scalable adapters-based channel handler operations.

…tions

Motivation:

ChannelDuplexHandler can implements both ChannelOutboundHandler and ChannelInboundHandler causing a scalability issue due to checkcast due to https://bugs.openjdk.org/browse/JDK-8180450
Not only: there are different classes eg Http2ConnectionHandler, which implement them transitively, by using one of the 2 existing adapters (ChannelInboundAdapter, ChanneOutboundAdapters).
The existing change at netty#12806 was fixing only the duplex cases, but others like the above one was still affected.

Modifications:

Replace the duplex type checks with broader inbound adapter ones, given that duplex is still based on it.
Add outbound adapters type checks in addition to duplex ones.

Result:

More scalable adapters-based channel handler operations.
@@ -653,6 +655,8 @@
headContext.connect(this, remoteAddress, localAddress, promise);
} else if (handler instanceof ChannelDuplexHandler) {
((ChannelDuplexHandler) handler).connect(this, remoteAddress, localAddress, promise);
} else if (handler instanceof ChannelOutboundHandlerAdapter) {
((ChannelOutboundHandlerAdapter) handler).connect(this, remoteAddress, localAddress, promise);

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

Potential server-side request forgery due to a
user-provided value
.
Potential server-side request forgery due to a
user-provided value
.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CodeQL go home...you're drunk :"(

@franz1981
Copy link
Contributor Author

franz1981 commented Dec 19, 2023

This mitigate eclipse-vertx/vert.x#5039 and help any case where we have adapters which implement the missing out/in handler.
To better explain, if we have a ping-pong of secondary_super_cache's value among 2 interfaces, by removing it happening to just one of them (eg the adapter it extends), we automatically solve the other, which become a secondary_super_cache's hit, instead.

ATM the other solutions doesn't look any better:

  1. singularly fix the affected "leaf" class(es) eg Http2ConnectionHandler and other listed at Fix scalability issue due to checkcast on context's invoke operations #12806 (comment) "somehow": eg turning them into duplex handlers which compose with the types they are extending
  2. fix key intermediate classes eg ByteToMessageDecoders, MessageToByteEncoders which can be used as building blocks - making them duplex with nop (probably it would break Netty...)
  3. create a "god abstract superclass" (or interface too!) which contains methods like these
ChannelOutBoundHandler asChannelOutbound() { 
    return this 
}

ChannelOutBoundHandler asChannelOutbound() { 
    return null 
}

which could be used within Netty's as a single instanceof GodNettyHandlerClass to safely request the interface types it implements by using the asXYZ method (which doesn't perform any cast): if these methods won't be overridden they will likely resolve in a cheap monomorphic call which doesn't contains any checkcast.

And obviously, the other solution is the one I've made here :/

@franz1981
Copy link
Contributor Author

Sorry that i didn't make it the first time like this @normanmaurer but after one year (or more) fixing these around in different libraries we've learnt different ways to improve the solutions, which are still ugly, at best

@franz1981 franz1981 changed the title Redo fix scalability issue due to checkcast on context's invoke opera… Redo fix scalability issue due to checkcast on context's invoke operation Dec 19, 2023
@franz1981 franz1981 changed the title Redo fix scalability issue due to checkcast on context's invoke operation Redo fix scalability issue due to checkcast on context's invoke operations Dec 19, 2023
@normanmaurer
Copy link
Member

@franz1981 no worries… Thanks for the work!

@normanmaurer normanmaurer merged commit 427ace4 into netty:4.1 Dec 19, 2023
15 checks passed
@normanmaurer
Copy link
Member

@franz1981 do you have any numbers for before and after ?

@normanmaurer normanmaurer added this to the 4.1.105.Final milestone Dec 19, 2023
@franz1981
Copy link
Contributor Author

franz1981 commented Dec 19, 2023

@normanmaurer I'm running other checks with some nightly run, mostly to be sure no regressions due to the additional type checks, TBH, but the first tests reported the usual ~15%-60% performance increase (is not as bad as the http one) based on the number of cores. I know it sounds wow, but is really a bug, which shouldn't surprise.

I can modify the existing io.netty.microbench.channel.DefaultChannelPipelineHandlerScalabilityBenchmark to cover the 2 additional cases, wdyt? This help having an easier way to trigger it

@normanmaurer
Copy link
Member

sounds good

@franz1981
Copy link
Contributor Author

Based on some nightly run, I see no regressions, meaning that, if it works, it works great (fixing the scalability issue, hence delivering staggering improvements for CPU bound scenarios with many cores), if it doesn't, no harm.
Funny things we do have improvements in some special unexpected cases, because peeling off the call site of handler's call and duplicating them can improve the chance of monomorphic/bimorphic call-sites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants