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.

Sign up for GitHub

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

Jump to bottom

Save promises type pollution due to interface type checks #12980

Merged
merged 2 commits into from Nov 29, 2022
Merged

Save promises type pollution due to interface type checks #12980

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91d137f
Save promises type pollution due to interface type checks
franz1981 Nov 9, 2022
465a599
Apply suggestions from code review
normanmaurer Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
75 changes: 48 additions & 27 deletions common/src/main/java/io/netty/util/concurrent/DefaultPromise.java
View file
Open in desktop
Expand Up @@ -55,7 +55,8 @@ public class DefaultPromise<V> extends AbstractFuture<V> implements Promise<V> {
*
* Threading - synchronized(this). We must support adding listeners when there is no EventExecutor.
*/
private Object listeners;
private GenericFutureListener<? extends Future<?>> listener;
private DefaultFutureListeners listeners;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/**
* Threading - synchronized(this). We are required to hold the monitor to use Java's underlying wait()/notifyAll().
*/
Expand Down Expand Up @@ -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;
}
}
}
}
Expand All @@ -584,20 +596,28 @@ private static void notifyListener0(Future future, GenericFutureListener l) {
}

private void addListener0(GenericFutureListener<? extends Future<? super V>> 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<? extends Future<? super V>> listener) {
if (listeners instanceof DefaultFutureListeners) {
((DefaultFutureListeners) listeners).remove(listener);
} else if (listeners == listener) {
listeners = null;
private void removeListener0(GenericFutureListener<? extends Future<? super V>> toRemove) {
if (listener == toRemove) {
listener = null;
} else if (listeners != null) {
listeners.remove(toRemove);
// Removal is rare, no need for compaction
if (listeners.size() == 0) {
listeners = null;
}
}
}

Expand Down Expand Up @@ -628,7 +648,7 @@ private synchronized boolean checkNotifyWaiters() {
if (waiters > 0) {
notifyAll();
}
return listeners != null;
return listener != null || listeners != null;
}

private void incWaiters() {
Expand Down Expand Up @@ -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:
Expand All @@ -791,8 +812,8 @@ private synchronized Object progressiveListeners() {
}

return copy;
} else if (listeners instanceof GenericProgressiveFutureListener) {
return listeners;
} else if (listener instanceof GenericProgressiveFutureListener) {
return listener;
} else {
// Only one listener was added and it's not a progressive listener.
return null;
Expand Down