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

Save promises type pollution due to interface type checks #12980

Merged
merged 2 commits into from Nov 29, 2022
Merged
Changes from 1 commit
Commits
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
76 changes: 49 additions & 27 deletions common/src/main/java/io/netty/util/concurrent/DefaultPromise.java
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);
// TODO we need ot compact it?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

normanmaurer marked this conversation as resolved.
Show resolved Hide resolved
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,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?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

normanmaurer marked this conversation as resolved.
Show resolved Hide resolved
return listener;
} else {
// Only one listener was added and it's not a progressive listener.
return null;
Expand Down