Repository: netty/netty
Issue: Http 2 type check slow path detected
Labels: improvement

This is the `MISS` report from a load test running against a HTTP 2 server written in Netty (basically an hello world-like case, really):

```
--------------------------
Miss:
--------------------------
1:      io.netty.handler.codec.http2.Http2CodecUtil$SimpleChannelPromiseAggregator
Count:  208210860
Types:
        io.netty.channel.ChannelProgressivePromise
Traces:
        io.netty.channel.ChannelOutboundBuffer.progress(ChannelOutboundBuffer.java:255)
                class: io.netty.channel.ChannelProgressivePromise
                count: 208210860
--------------------------
2:      io.netty.handler.timeout.IdleStateHandler$1
Count:  187763163
Types:
        io.netty.util.concurrent.GenericProgressiveFutureListener
Traces:
        io.netty.util.concurrent.DefaultFutureListeners.<init>(DefaultFutureListeners.java:36)
                class: io.netty.util.concurrent.GenericProgressiveFutureListener
                count: 95034173
        io.netty.util.concurrent.DefaultFutureListeners.<init>(DefaultFutureListeners.java:33)
                class: io.netty.util.concurrent.GenericProgressiveFutureListener
                count: 92728990
--------------------------
3:      io.netty.channel.DelegatingChannelPromiseNotifier
Count:  46112141
Types:
        io.netty.util.concurrent.GenericProgressiveFutureListener
Traces:
        io.netty.util.concurrent.DefaultFutureListeners.<init>(DefaultFutureListeners.java:36)
                class: io.netty.util.concurrent.GenericProgressiveFutureListener
                count: 46112141
--------------------------
4:      io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder$1
Count:  42734531
Types:
        io.netty.util.concurrent.GenericProgressiveFutureListener
Traces:
        io.netty.util.concurrent.DefaultFutureListeners.add(DefaultFutureListeners.java:50)
                class: io.netty.util.concurrent.GenericProgressiveFutureListener
                count: 42734531
--------------------------
5:      io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder$FlowControlledData
Count:  42284283
Types:
        io.netty.util.concurrent.GenericProgressiveFutureListener
Traces:
        io.netty.util.concurrent.DefaultFutureListeners.<init>(DefaultFutureListeners.java:33)
                class: io.netty.util.concurrent.GenericProgressiveFutureListener
                count: 42284283
--------------------------
6:      io.netty.handler.codec.http2.Http2ConnectionHandler$2
Count:  41044039
Types:
        io.netty.util.concurrent.GenericProgressiveFutureListener
Traces:
        io.netty.util.concurrent.DefaultFutureListeners.<init>(DefaultFutureListeners.java:36)
                class: io.netty.util.concurrent.GenericProgressiveFutureListener
                count: 41044039
--------------------------
7:      io.netty.channel.VoidChannelPromise$1
Count:  40372959
Types:
        io.netty.util.concurrent.GenericProgressiveFutureListener
Traces:
        io.netty.util.concurrent.DefaultFutureListeners.<init>(DefaultFutureListeners.java:33)
                class: io.netty.util.concurrent.GenericProgressiveFutureListener
                count: 40372959
--------------------------
8:      io.netty.buffer.UnpooledSlicedByteBuf
Count:  40123310
Types:
        io.netty.channel.ChannelFutureListener
Traces:
        io.netty.channel.AbstractCoalescingBufferQueue.remove(AbstractCoalescingBufferQueue.java:157)
                class: io.netty.channel.ChannelFutureListener
                count: 40123310
--------------------------
9:      io.netty.buffer.PooledUnsafeDirectByteBuf
Count:  5541640
Types:
        io.netty.handler.codec.http2.Http2StreamFrame
Traces:
        io.vertx.core.http.impl.VertxHttp2ConnectionHandler.channelRead(VertxHttp2ConnectionHandler.java:405)
                class: io.netty.handler.codec.http2.Http2StreamFrame
                count: 5541640
--------------------------

```

The report is compatible with Idea stack analyzer, meaning that can be copied & pasted there and just click in the traces which report the line-of-codes, to address the problems.

An explanation about what the report is and why is it worthy to fix the issues it reports.

As explained by https://github.com/RedHatPerf/type-pollution-agent misusing interfaces type checks can cause scalability problems, but there are other problems which can happen on how OpenJDK handle the "slow-path" of interface type checks
ie if a concrete type doesn't implement a specific interface and, in the hot path, the user code perform a type check against it:
in this case the JIT perform a O(n) search (using some vectorized instructions for x86, the infamous `prnz scas`) among the interfaces it transitively implements (so called `secondary_supers`) just to find that the type check would fail.
This is  obviously not as bad as https://bugs.openjdk.org/browse/JDK-8180450 but it still pretty bad, depending how hot the code path is and how many interfaces are implemented.

While developing the type pollution agent, I've found this behaviour and reported it to https://ionutbalosin.com/2023/03/jvm-performance-comparison-for-jdk-17/#typecheckbenchmark to better understand if others JIT impl are affected.

Sadly, at the moment of filling this issue, I'm not aware if @rwestrel patch at https://github.com/openjdk/jdk/pull/14375 (which should be in JDK 22 AFAIK), save it the day and fix it, but for sure < JDK 22, even in case of monomorphic receivers (even statically proven as monomorphic) the slow type check always happen.

The agent has been than enhanced to capture these cases and it should be read/use like this

eg

```
--------------------------
Miss:
--------------------------
1:      io.netty.handler.codec.http2.Http2CodecUtil$SimpleChannelPromiseAggregator
Count:  208210860
Types:
        io.netty.channel.ChannelProgressivePromise
Traces:
        io.netty.channel.ChannelOutboundBuffer.progress(ChannelOutboundBuffer.java:255)
                class: io.netty.channel.ChannelProgressivePromise
                count: 208210860
--------------------------
```
it means that `SimpleChannelPromiseAggregator` is being tested against `ChannelProgressivePromise` interface +200 M times, while not implementing it at

```java
    public void progress(long amount) {
        Entry e = flushedEntry;
        assert e != null;
        ChannelPromise p = e.promise;
        long progress = e.progress + amount;
        e.progress = progress;
        assert p != null;
        final Class<?> promiseClass = p.getClass();
        // fast-path to save O(n) ChannelProgressivePromise's type check on OpenJDK
        if (promiseClass == VoidChannelPromise.class || promiseClass == DefaultChannelPromise.class) {
            return;
        }
        // this is going to save from type pollution due to https://bugs.openjdk.org/browse/JDK-8180450
        if (p instanceof DefaultChannelProgressivePromise) {
            ((DefaultChannelProgressivePromise) p).tryProgress(progress, e.total);
        } else if (p instanceof ChannelProgressivePromise) {              <------- HERE!
            ((ChannelProgressivePromise) p).tryProgress(progress, e.total);
        }
    }
```
at https://github.com/netty/netty/blob/5155482afc57907a1414994ef5aca79b61ac6999/transport/src/main/java/io/netty/channel/ChannelOutboundBuffer.java#L255

And we know that there was already a fix to speedup the O(n) search against `DefaultChannelPromise`'s exact class, which is indeed the type extended by `SimpleChannelPromiseAggregator`.

Can we fix it just by changing
```java
        // fast-path to save O(n) ChannelProgressivePromise's type check on OpenJDK
        if (promiseClass == VoidChannelPromise.class || promiseClass == DefaultChannelPromise.class) {
            return;
        }
```
into
```java
       // fast-path to save O(n) ChannelProgressivePromise's type check on OpenJDK
        if (promiseClass == VoidChannelPromise.class || promiseClass instanceof DefaultChannelPromise) {
            return;
        }
```
?
Sadly not, because we cannot trust that extending `DefaultChannelPromise` means NOT implementing `ChannelProgressivePromise` (the original change happened at https://github.com/netty/netty/pull/13225)

At the same time a check directly against the `SimpleChannelPromiseAggregator`  class introduce the dependency of http2 in a core part, which is just not desiderable/possible.

So, how to fix it? 
And, is it worthy?

The former got no precise answer: it depends, and i'm opened to suggestions here.
The latter is, usually yes, but your mileage may vary: sadly most Java profilers breaks on it, so it's not easy to find it out, till fixing it.

Issue comments:
I've verified via https://github.com/franz1981/java-puzzles/commit/e083daa22878511c475135b5863b861471e617a6 that thanks to https://github.com/openjdk/jdk/pull/14375 the type check performed at https://github.com/netty/netty/blob/5155482afc57907a1414994ef5aca79b61ac6999/transport/src/main/java/io/netty/channel/ChannelOutboundBuffer.java#L255 can add a bimorphic guard with at best 2 concrete types before hitting the O(n) slow path type check, ,making this issue less impactful, but still showing that knowing the arity of the type checks receivers and trying reducing it via type peeling off (eg instanceof vs a known hot path concrete type) can help the JIT to have enough "budget" for the remaining ones, and make the JIT optimization even more effective.

Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java b/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java
index 20a0ba09f7b..33f3d81d8e1 100644
--- a/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java
+++ b/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java
@@ -154,38 +154,40 @@ public final ByteBuf remove(ByteBufAllocator alloc, int bytes, ChannelPromise ag
                 if (entry == null) {
                     break;
                 }
-                if (entry instanceof ChannelFutureListener) {
-                    aggregatePromise.addListener((ChannelFutureListener) entry);
-                    continue;
-                }
-
-                entryBuffer = (ByteBuf) entry;
-                int bufferBytes = entryBuffer.readableBytes();
-
-                if (bufferBytes > bytes) {
-                    // Add the buffer back to the queue as we can't consume all of it.
-                    bufAndListenerPairs.addFirst(entryBuffer);
-                    if (bytes > 0) {
-                        // Take a slice of what we can consume and retain it.
-                        entryBuffer = entryBuffer.readRetainedSlice(bytes);
-                        // we end here, so if this is the only buffer to return, skip composing
-                        toReturn = toReturn == null ? entryBuffer
-                                                    : compose(alloc, toReturn, entryBuffer);
-                        bytes = 0;
+                // fast-path vs abstract type
+                if (entry instanceof ByteBuf) {
+                    entryBuffer = (ByteBuf) entry;
+                    int bufferBytes = entryBuffer.readableBytes();
+
+                    if (bufferBytes > bytes) {
+                        // Add the buffer back to the queue as we can't consume all of it.
+                        bufAndListenerPairs.addFirst(entryBuffer);
+                        if (bytes > 0) {
+                            // Take a slice of what we can consume and retain it.
+                            entryBuffer = entryBuffer.readRetainedSlice(bytes);
+                            // we end here, so if this is the only buffer to return, skip composing
+                            toReturn = toReturn == null ? entryBuffer
+                                    : compose(alloc, toReturn, entryBuffer);
+                            bytes = 0;
+                        }
+                        break;
                     }
-                    break;
-                }
 
-                bytes -= bufferBytes;
-                if (toReturn == null) {
-                    // if there are no more bytes in the queue after this, there's no reason to compose
-                    toReturn = bufferBytes == readableBytes
-                            ? entryBuffer
-                            : composeFirst(alloc, entryBuffer);
-                } else {
-                    toReturn = compose(alloc, toReturn, entryBuffer);
+                    bytes -= bufferBytes;
+                    if (toReturn == null) {
+                        // if there are no more bytes in the queue after this, there's no reason to compose
+                        toReturn = bufferBytes == readableBytes
+                                ? entryBuffer
+                                : composeFirst(alloc, entryBuffer);
+                    } else {
+                        toReturn = compose(alloc, toReturn, entryBuffer);
+                    }
+                    entryBuffer = null;
+                } else if (entry instanceof DelegatingChannelPromiseNotifier) {
+                    aggregatePromise.addListener((DelegatingChannelPromiseNotifier) entry);
+                } else if (entry instanceof ChannelFutureListener) {
+                    aggregatePromise.addListener((ChannelFutureListener) entry);
                 }
-                entryBuffer = null;
             }
         } catch (Throwable cause) {
             safeRelease(entryBuffer);


Test patch: