Skip to content

Commit

Permalink
DefaultPromise make listeners not volatile
Browse files Browse the repository at this point in the history
Motivation:
DefaultPromise has a listeners member variable which is volatile to allow for an optimization which makes notification of listeners less expensive when there are no listeners to notify. However this change makes all other operations involving the listeners member variable more costly. This optimization which requires listeners to be volatile can be removed to avoid volatile writes/reads for every access on the listeners member variable.

Modifications:
- DefaultPromise listeners is made non-volatile and the null check optimization is removed

Result:
DefaultPromise.listeners is no longer volatile.
  • Loading branch information
Scottmitch authored and normanmaurer committed Jul 7, 2016
1 parent e3c8a92 commit 4d8132f
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.netty.util.ReferenceCountUtil;
import io.netty.util.concurrent.EventExecutor;
import io.netty.util.concurrent.GenericFutureListener;
import io.netty.util.concurrent.ImmediateEventExecutor;
import io.netty.util.concurrent.Promise;
import org.junit.After;
import org.junit.Before;
Expand Down Expand Up @@ -124,7 +125,7 @@ public class Http2ConnectionHandlerTest {
public void setup() throws Exception {
MockitoAnnotations.initMocks(this);

promise = new DefaultChannelPromise(channel);
promise = new DefaultChannelPromise(channel, ImmediateEventExecutor.INSTANCE);

Throwable fakeException = new RuntimeException("Fake exception");
when(encoder.connection()).thenReturn(connection);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public class DefaultPromise<V> extends AbstractFuture<V> implements Promise<V> {
*
* Threading - synchronized(this). We must support adding listeners when there is no EventExecutor.
*/
private volatile Object listeners;
private Object listeners;
/**
* Threading - synchronized(this). We are required to hold the monitor to use Java's underlying wait()/notifyAll().
*/
Expand Down Expand Up @@ -417,13 +417,6 @@ protected static void notifyListener(
}

private void notifyListeners() {
if (listeners == null) {
return;
}
notifyListenersWithStackOverFlowProtection();
}

private void notifyListenersWithStackOverFlowProtection() {
EventExecutor executor = executor();
if (executor.inEventLoop()) {
final InternalThreadLocalMap threadLocals = InternalThreadLocalMap.get();
Expand All @@ -448,7 +441,7 @@ public void run() {
}

/**
* The logic in this method should be identical to {@link #notifyListenersWithStackOverFlowProtection()} but
* The logic in this method should be identical to {@link #notifyListeners()} but
* cannot share code because the listener(s) cannot be cached for an instance of {@link DefaultPromise} since the
* listener(s) may be changed and is protected by a synchronized operation.
*/
Expand Down

0 comments on commit 4d8132f

Please sign in to comment.