From a6abff75a6fe779a1b866424049372bbbc1a7fb3 Mon Sep 17 00:00:00 2001 From: Nick Hill Date: Fri, 12 Jul 2019 09:06:26 -0700 Subject: [PATCH] Epoll: Don't wake event loop when splicing (#9354) Motivation I noticed this while looking at something else. AbstractEpollStreamChannel::spliceQueue is an MPSC queue but only accessed from the event loop. So it could be just changed to e.g. an ArrayDeque. This PR instead reverts to using is as an MPSC queue to avoid dispatching a task to the EL, as appears was the original intention. Modification Change AbstractEpollStreamChannel::spliceQueue to be volatile and lazily initialized via double-checked locking. Add tasks directly to the queue from the public methods rather than possibly waking the EL just to enqueue. An alternative is just to change PlatformDependent.newMpscQueue() to new ArrayDeque() and be done with it :) Result Less disruptive channel/fd-splicing. --- .../epoll/AbstractEpollStreamChannel.java | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java b/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java index 6a48ea8c14..71abae94cb 100644 --- a/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java +++ b/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java @@ -66,9 +66,9 @@ public abstract class AbstractEpollStreamChannel extends AbstractEpollChannel im // meantime. ((AbstractEpollUnsafe) unsafe()).flush0(); }; - private Queue spliceQueue; // Lazy init these if we need to splice(...) + private volatile Queue spliceQueue; private FileDescriptor pipeIn; private FileDescriptor pipeOut; @@ -646,13 +646,14 @@ public abstract class AbstractEpollStreamChannel extends AbstractEpollChannel im } private void clearSpliceQueue() { - if (spliceQueue == null) { + Queue sQueue = spliceQueue; + if (sQueue == null) { return; } ClosedChannelException exception = null; for (;;) { - SpliceInTask task = spliceQueue.poll(); + SpliceInTask task = sQueue.poll(); if (task == null) { break; } @@ -725,15 +726,16 @@ public abstract class AbstractEpollStreamChannel extends AbstractEpollChannel im ByteBuf byteBuf = null; boolean close = false; try { + Queue sQueue = null; do { - if (spliceQueue != null) { - SpliceInTask spliceTask = spliceQueue.peek(); + if (sQueue != null || (sQueue = spliceQueue) != null) { + SpliceInTask spliceTask = sQueue.peek(); if (spliceTask != null) { if (spliceTask.spliceIn(allocHandle)) { // We need to check if it is still active as if not we removed all SpliceTasks in // doClose(...) if (isActive()) { - spliceQueue.remove(); + sQueue.remove(); } continue; } else { @@ -795,19 +797,16 @@ public abstract class AbstractEpollStreamChannel extends AbstractEpollChannel im } private void addToSpliceQueue(final SpliceInTask task) { - EventLoop eventLoop = eventLoop(); - if (eventLoop.inEventLoop()) { - addToSpliceQueue0(task); - } else { - eventLoop.execute(() -> addToSpliceQueue0(task)); + Queue sQueue = spliceQueue; + if (sQueue == null) { + synchronized (this) { + sQueue = spliceQueue; + if (sQueue == null) { + spliceQueue = sQueue = PlatformDependent.newMpscQueue(); + } + } } - } - - private void addToSpliceQueue0(SpliceInTask task) { - if (spliceQueue == null) { - spliceQueue = PlatformDependent.newMpscQueue(); - } - spliceQueue.add(task); + sQueue.add(task); } protected abstract class SpliceInTask {