Save promises type pollution due to interface type checks #12980

franz1981 · 2022-11-09T08:50:47Z

Motivation:

DefaultPromise performs interfaces type checks to distinguish single/multi listeners presence, hitting https://bugs.openjdk.org/browse/JDK-8180450

Modifications:

Using separate listener fields that won't require type checks to evaluate listeners arity

Result:
No type pollution for user-defined listeners types.

franz1981 · 2022-11-09T09:07:38Z

FYI, the relevant guilty part is the cast to GenericFutureListener<?>: if users implements their own promises/listeners as a single class, they'll be bitten by how PromiseImpl works:

3:  io.vertx.core.impl.future.PromiseImpl
Count:  9035522
Types:
    io.vertx.core.AsyncResult
    io.vertx.core.impl.future.PromiseInternal
    io.netty.util.concurrent.GenericFutureListener
    io.vertx.core.impl.future.Listener
    io.vertx.core.Future
    io.vertx.core.Promise
Traces:
    io.vertx.core.Promise.handle(Promise.java:29)
        class: io.vertx.core.AsyncResult
        count: 4180290
    io.vertx.core.impl.ContextInternal.promise(ContextInternal.java:84)
        class: io.vertx.core.impl.future.PromiseInternal
        count: 3425143
    io.netty.util.concurrent.DefaultPromise.addListener0(DefaultPromise.java:592)
        class: io.netty.util.concurrent.GenericFutureListener
        count: 990012
    io.vertx.core.impl.ContextInternal.promise(ContextInternal.java:85)
        class: io.vertx.core.impl.future.PromiseInternal
        count: 438087
    io.vertx.sqlclient.impl.PoolImpl.lambda$new$0(PoolImpl.java:73)
        class: io.vertx.core.Future
        count: 614
    io.vertx.core.impl.future.SucceededFuture.onComplete(SucceededFuture.java:76)
        class: io.vertx.core.impl.future.Listener
        count: 486
    io.vertx.core.impl.future.FutureImpl.onComplete(FutureImpl.java:134)
        class: io.vertx.core.impl.future.Listener
        count: 413
    io.netty.util.concurrent.DefaultPromise.notifyListenersNow(DefaultPromise.java:552)
        class: io.netty.util.concurrent.GenericFutureListener
        count: 289
    io.quarkus.scheduler.runtime.SimpleScheduler$ScheduledTask$1.handle(SimpleScheduler.java:310)
        class: io.vertx.core.Promise
        count: 81
    io.vertx.core.impl.future.FutureImpl.onComplete(FutureImpl.java:135)
        class: io.vertx.core.impl.future.Listener
        count: 42
    io.vertx.sqlclient.impl.pool.SqlConnectionPool$2.connect(SqlConnectionPool.java:107)
        class: io.vertx.core.Future
        count: 34
    io.vertx.core.impl.future.SucceededFuture.onComplete(SucceededFuture.java:77)
        class: io.vertx.core.impl.future.Listener
        count: 31

Adding a new field won't impact instance footprint

before

io.netty.util.concurrent.DefaultPromise object internals:
OFF  SZ            TYPE DESCRIPTION                         VALUE
  0   8                 (object header: mark)               N/A
  8   4                 (object header: class)              N/A
 12   2           short DefaultPromise.waiters              N/A
 14   1         boolean DefaultPromise.notifyingListeners   N/A
 15   1                 (alignment/padding gap)             
 16   4          Object DefaultPromise.result               N/A
 20   4   EventExecutor DefaultPromise.executor             N/A
 24   4          Object DefaultPromise.listeners            N/A
 28   4                 (object alignment gap)              
Instance size: 32 bytes
Space losses: 1 bytes internal + 4 bytes external = 5 bytes total

After:

io.netty.util.concurrent.DefaultPromise object internals:
OFF  SZ                                         TYPE DESCRIPTION                         VALUE
  0   8                                              (object header: mark)               N/A
  8   4                                              (object header: class)              N/A
 12   2                                        short DefaultPromise.waiters              N/A
 14   1                                      boolean DefaultPromise.notifyingListeners   N/A
 15   1                                              (alignment/padding gap)             
 16   4                                       Object DefaultPromise.result               N/A
 20   4                                EventExecutor DefaultPromise.executor             N/A
 24   4   GenericFutureListener<? extends Future<?>> DefaultPromise.listener             N/A
 28   4                       DefaultFutureListeners DefaultPromise.listeners            N/A
Instance size: 32 bytes
Space losses: 1 bytes internal + 0 bytes external = 1 bytes total

Thanks to pad, the added field won't cost us an increased mem footprint.
This issue isn't directly a Netty fault, but saving useless type checks just makes user-code less prone to the mentioned JDK issue

Motivation: DefaultPromise performs interfaces type checks to distinguish single/multi listeners presence, hitting https://bugs.openjdk.org/browse/JDK-8180450 Modifications: Using separate listener fields that won't require type checks to evaluate listeners arity Result: No type pollution for user-defined listeners types.

franz1981 · 2022-11-09T09:32:30Z

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java

+            listener = null;
+        } else if (listeners != null) {
+            listeners.remove(toRemove);
+            // TODO we need ot compact it?


@normanmaurer
The compaction can happen while moving from 2 to 1 element too, but given that the listeners collection is already allocated won't worths

Yeah, let's not bother with compaction. Removing listeners is so rare, and as you say, we already allocated the collection. Not to mention, if listeners are repeatedly added and removed, we don't want to thrash between the two modes and produce garbage on every switch.

franz1981 · 2022-11-09T09:33:07Z

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java

-        } else if (listeners instanceof GenericProgressiveFutureListener) {
-            return listeners;
+        } else if (listener instanceof GenericProgressiveFutureListener) {
+            // TODO can we save the instanceof and just makes use of the progressiveSize?


@normanmaurer ^^

Progressive promises and futures are not used very often, so fine to not optimize IMO.

franz1981 · 2022-11-09T09:33:44Z

The PR is ready to be reviewed, is still on draft because I'm waiting my collagues to test is in a proper end 2 end test

normanmaurer · 2022-11-09T14:17:27Z

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java

@@ -55,7 +55,8 @@
     *
     * Threading - synchronized(this). We must support adding listeners when there is no EventExecutor.
     */
-    private Object listeners;
+    private GenericFutureListener<? extends Future<?>> listener;
+    private DefaultFutureListeners listeners;


FYI we used Object to be able to save space in the past. That said I think this is not a good reason for this anymore

eheh I believe so: I've reported on #12980 (comment) the actual size by using this super nice IDEA plugin: https://plugins.jetbrains.com/plugin/10953-jol-java-object-layout

franz1981 · 2022-11-24T11:20:48Z

@normanmaurer how I can trigger again new tests? I don't know if I got such super powers

normanmaurer · 2022-11-24T11:23:05Z

@franz1981 do you see the "re-run tests button on the right top corner" ? https://github.com/netty/netty/actions/runs/3426801410/jobs/5709114178

franz1981 · 2022-11-24T12:58:26Z

thanks @normanmaurer , yep! rerun now

franz1981 · 2022-11-24T20:48:32Z

Now CI looks much better :)
As said, this change is important for users that define their own types, but should still deliver an improvement on Netty as well although not because of the usual infamous scalability JDK bug: instanceof was used to distinguish between single/mutiple listeners and its cost, in the slow path, means O(n) search between the transitively implemented interfaces, while now, at worst, is a single variable null-check.

@normanmaurer I've placed some TODO to get feedbacks on other code paths, let me know if I can rid of them and open a separate pr instead, to move this forward

chrisvest

Had opinions on the TODOs, but otherwise looks good.

chrisvest · 2022-11-28T23:10:29Z

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java

+            listener = null;
+        } else if (listeners != null) {
+            listeners.remove(toRemove);
+            // TODO we need ot compact it?


Yeah, let's not bother with compaction. Removing listeners is so rare, and as you say, we already allocated the collection. Not to mention, if listeners are repeatedly added and removed, we don't want to thrash between the two modes and produce garbage on every switch.

chrisvest · 2022-11-28T23:15:30Z

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java

-        } else if (listeners instanceof GenericProgressiveFutureListener) {
-            return listeners;
+        } else if (listener instanceof GenericProgressiveFutureListener) {
+            // TODO can we save the instanceof and just makes use of the progressiveSize?


Progressive promises and futures are not used very often, so fine to not optimize IMO.

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java

franz1981 requested review from chrisvest and normanmaurer November 9, 2022 08:50

franz1981 self-assigned this Nov 9, 2022

franz1981 force-pushed the listener_type_pollution branch from 53b46fb to fc24a74 Compare November 9, 2022 09:11

franz1981 force-pushed the listener_type_pollution branch from fc24a74 to 91d137f Compare November 9, 2022 09:12

franz1981 commented Nov 9, 2022

View reviewed changes

normanmaurer reviewed Nov 9, 2022

View reviewed changes

normanmaurer approved these changes Nov 11, 2022

View reviewed changes

franz1981 marked this pull request as ready for review November 23, 2022 20:15

chrisvest approved these changes Nov 28, 2022

View reviewed changes

normanmaurer approved these changes Nov 28, 2022

View reviewed changes

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java Outdated Show resolved Hide resolved

common/src/main/java/io/netty/util/concurrent/DefaultPromise.java Outdated Show resolved Hide resolved

Apply suggestions from code review

465a599

normanmaurer merged commit 22d3151 into netty:4.1 Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save promises type pollution due to interface type checks #12980

Save promises type pollution due to interface type checks #12980

franz1981 commented Nov 9, 2022

franz1981 commented Nov 9, 2022 •

edited

franz1981 Nov 9, 2022

chrisvest Nov 28, 2022

franz1981 Nov 9, 2022

chrisvest Nov 28, 2022

franz1981 commented Nov 9, 2022

normanmaurer Nov 9, 2022

franz1981 Nov 10, 2022

franz1981 commented Nov 24, 2022

normanmaurer commented Nov 24, 2022

franz1981 commented Nov 24, 2022

franz1981 commented Nov 24, 2022 •

edited

chrisvest left a comment

chrisvest Nov 28, 2022

chrisvest Nov 28, 2022

Save promises type pollution due to interface type checks #12980

Save promises type pollution due to interface type checks #12980

Conversation

franz1981 commented Nov 9, 2022

franz1981 commented Nov 9, 2022 • 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

franz1981 commented Nov 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franz1981 commented Nov 24, 2022

normanmaurer commented Nov 24, 2022

franz1981 commented Nov 24, 2022

franz1981 commented Nov 24, 2022 • edited

chrisvest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franz1981 commented Nov 9, 2022 •

edited

franz1981 commented Nov 24, 2022 •

edited