From 91d137f043dd78efda132bbd4222c85c8e8d1d6a Mon Sep 17 00:00:00 2001 From: franz1981 Date: Wed, 9 Nov 2022 09:49:06 +0100 Subject: [PATCH 1/2] Save promises type pollution due to interface type checks 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. --- .../netty/util/concurrent/DefaultPromise.java | 76 ++++++++++++------- 1 file changed, 49 insertions(+), 27 deletions(-) diff --git a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java index 9aa503ae350..d6044899c85 100644 --- a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java +++ b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java @@ -55,7 +55,8 @@ public class DefaultPromise extends AbstractFuture implements Promise { * * Threading - synchronized(this). We must support adding listeners when there is no EventExecutor. */ - private Object listeners; + private GenericFutureListener> listener; + private DefaultFutureListeners listeners; /** * Threading - synchronized(this). We are required to hold the monitor to use Java's underlying wait()/notifyAll(). */ @@ -535,31 +536,42 @@ public void run() { } private void notifyListenersNow() { - Object listeners; + GenericFutureListener listener; + DefaultFutureListeners listeners; synchronized (this) { + listener = this.listener; + listeners = this.listeners; // Only proceed if there are listeners to notify and we are not already notifying listeners. - if (notifyingListeners || this.listeners == null) { + if (notifyingListeners || (listener == null && listeners == null)) { return; } notifyingListeners = true; - listeners = this.listeners; - this.listeners = null; + if (listener != null) { + this.listener = null; + } else { + this.listeners = null; + } } for (;;) { - if (listeners instanceof DefaultFutureListeners) { - notifyListeners0((DefaultFutureListeners) listeners); + if (listener != null) { + notifyListener0(this, listener); } else { - notifyListener0(this, (GenericFutureListener) listeners); + notifyListeners0(listeners); } synchronized (this) { - if (this.listeners == null) { + if (this.listener == null && this.listeners == null) { // Nothing can throw from within this method, so setting notifyingListeners back to false does not // need to be in a finally block. notifyingListeners = false; return; } + listener = this.listener; listeners = this.listeners; - this.listeners = null; + if (listener != null) { + this.listener = null; + } else { + this.listeners = null; + } } } } @@ -584,20 +596,28 @@ private static void notifyListener0(Future future, GenericFutureListener l) { } private void addListener0(GenericFutureListener> listener) { - if (listeners == null) { - listeners = listener; - } else if (listeners instanceof DefaultFutureListeners) { - ((DefaultFutureListeners) listeners).add(listener); + if (this.listener == null) { + if (listeners == null) { + this.listener = listener; + } else { + listeners.add(listener); + } } else { - listeners = new DefaultFutureListeners((GenericFutureListener) listeners, listener); + assert listeners == null; + listeners = new DefaultFutureListeners(this.listener, listener); + this.listener = null; } } - private void removeListener0(GenericFutureListener> listener) { - if (listeners instanceof DefaultFutureListeners) { - ((DefaultFutureListeners) listeners).remove(listener); - } else if (listeners == listener) { - listeners = null; + private void removeListener0(GenericFutureListener> toRemove) { + if (listener == toRemove) { + listener = null; + } else if (listeners != null) { + listeners.remove(toRemove); + // TODO we need ot compact it? + if (listeners.size() == 0) { + listeners = null; + } } } @@ -628,7 +648,7 @@ private synchronized boolean checkNotifyWaiters() { if (waiters > 0) { notifyAll(); } - return listeners != null; + return listener != null || listeners != null; } private void incWaiters() { @@ -759,15 +779,16 @@ public void run() { * {@code null}. */ private synchronized Object progressiveListeners() { - Object listeners = this.listeners; - if (listeners == null) { + final GenericFutureListener listener = this.listener; + final DefaultFutureListeners listeners = this.listeners; + if (listener == null && listeners == null) { // No listeners added return null; } - if (listeners instanceof DefaultFutureListeners) { + if (listeners != null) { // Copy DefaultFutureListeners into an array of listeners. - DefaultFutureListeners dfl = (DefaultFutureListeners) listeners; + DefaultFutureListeners dfl = listeners; int progressiveSize = dfl.progressiveSize(); switch (progressiveSize) { case 0: @@ -791,8 +812,9 @@ private synchronized Object progressiveListeners() { } return copy; - } 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? + return listener; } else { // Only one listener was added and it's not a progressive listener. return null; From 465a599d99adb5f8b5d566440d6fd6f536c4c3a2 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 28 Nov 2022 15:42:36 -0800 Subject: [PATCH 2/2] Apply suggestions from code review --- .../src/main/java/io/netty/util/concurrent/DefaultPromise.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java index d6044899c85..a7946a92da4 100644 --- a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java +++ b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java @@ -614,7 +614,7 @@ private void removeListener0(GenericFutureListener> listener = null; } else if (listeners != null) { listeners.remove(toRemove); - // TODO we need ot compact it? + // Removal is rare, no need for compaction if (listeners.size() == 0) { listeners = null; } @@ -813,7 +813,6 @@ private synchronized Object progressiveListeners() { return copy; } else if (listener instanceof GenericProgressiveFutureListener) { - // TODO can we save the instanceof and just makes use of the progressiveSize? return listener; } else { // Only one listener was added and it's not a progressive listener.