From 795f318c3c11ec0520e7acd963ad4b310c287c20 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Wed, 23 Nov 2016 13:50:42 -0800 Subject: [PATCH] Use a single array in SelectedSelectionKeySet Motivation: SelectedSelectionKeySet currently uses 2 arrays internally and users are expected to call flip() to access the underlying array and switch the active array. However we do not concurrently use 2 arrays at the same time and we can get away with using a single array if we are careful about when we reset the elements of the array. Modifications: - Introduce SelectedSelectionKeySetSelector which wraps a Selector and ensures we reset the underlying SelectedSelectionKeySet data structures before we select - The loop bounds in NioEventLoop#processSelectedKeysOptimized can be defined more precisely because we know the real size of the underlying array Result: Fixes https://github.com/netty/netty/issues/6058 --- .../netty/channel/nio/AbstractNioChannel.java | 2 +- .../io/netty/channel/nio/NioEventLoop.java | 60 ++++++-------- .../channel/nio/SelectedSelectionKeySet.java | 76 ++++++------------ .../nio/SelectedSelectionKeySetSelector.java | 80 +++++++++++++++++++ .../nio/SelectedSelectionKeySetTest.java | 66 +++++++++++++++ 5 files changed, 194 insertions(+), 90 deletions(-) create mode 100644 transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySetSelector.java create mode 100644 transport/src/test/java/io/netty/channel/nio/SelectedSelectionKeySetTest.java diff --git a/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java b/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java index f90f1207a5..f9ea574347 100644 --- a/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java +++ b/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java @@ -384,7 +384,7 @@ public abstract class AbstractNioChannel extends AbstractChannel { boolean selected = false; for (;;) { try { - selectionKey = javaChannel().register(eventLoop().selector, 0, this); + selectionKey = javaChannel().register(eventLoop().unwrappedSelector(), 0, this); return; } catch (CancelledKeyException e) { if (!selected) { diff --git a/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java b/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java index 76f3fc79d6..5845f73b1c 100644 --- a/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java +++ b/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java @@ -116,7 +116,8 @@ public final class NioEventLoop extends SingleThreadEventLoop { /** * The NIO {@link Selector}. */ - Selector selector; + private Selector selector; + private Selector unwrappedSelector; private SelectedSelectionKeySet selectedKeys; private final SelectorProvider provider; @@ -150,15 +151,14 @@ public final class NioEventLoop extends SingleThreadEventLoop { } private Selector openSelector() { - final Selector selector; try { - selector = provider.openSelector(); + unwrappedSelector = provider.openSelector(); } catch (IOException e) { throw new ChannelException("failed to open a new selector", e); } if (DISABLE_KEYSET_OPTIMIZATION) { - return selector; + return unwrappedSelector; } final SelectedSelectionKeySet selectedKeySet = new SelectedSelectionKeySet(); @@ -179,12 +179,12 @@ public final class NioEventLoop extends SingleThreadEventLoop { if (!(maybeSelectorImplClass instanceof Class) || // ensure the current selector implementation is what we can instrument. - !((Class) maybeSelectorImplClass).isAssignableFrom(selector.getClass())) { + !((Class) maybeSelectorImplClass).isAssignableFrom(unwrappedSelector.getClass())) { if (maybeSelectorImplClass instanceof Throwable) { Throwable t = (Throwable) maybeSelectorImplClass; - logger.trace("failed to instrument a special java.util.Set into: {}", selector, t); + logger.trace("failed to instrument a special java.util.Set into: {}", unwrappedSelector, t); } - return selector; + return unwrappedSelector; } final Class selectorImplClass = (Class) maybeSelectorImplClass; @@ -205,8 +205,8 @@ public final class NioEventLoop extends SingleThreadEventLoop { return cause; } - selectedKeysField.set(selector, selectedKeySet); - publicSelectedKeysField.set(selector, selectedKeySet); + selectedKeysField.set(unwrappedSelector, selectedKeySet); + publicSelectedKeysField.set(unwrappedSelector, selectedKeySet); return null; } catch (NoSuchFieldException e) { return e; @@ -219,13 +219,12 @@ public final class NioEventLoop extends SingleThreadEventLoop { if (maybeException instanceof Exception) { selectedKeys = null; Exception e = (Exception) maybeException; - logger.trace("failed to instrument a special java.util.Set into: {}", selector, e); - } else { - selectedKeys = selectedKeySet; - logger.trace("instrumented a special java.util.Set into: {}", selector); + logger.trace("failed to instrument a special java.util.Set into: {}", unwrappedSelector, e); + return unwrappedSelector; } - - return selector; + selectedKeys = selectedKeySet; + logger.trace("instrumented a special java.util.Set into: {}", unwrappedSelector); + return new SelectedSelectionKeySetSelector(unwrappedSelector, selectedKeySet); } /** @@ -474,7 +473,7 @@ public final class NioEventLoop extends SingleThreadEventLoop { private void processSelectedKeys() { if (selectedKeys != null) { - processSelectedKeysOptimized(selectedKeys.flip()); + processSelectedKeysOptimized(); } else { processSelectedKeysPlain(selector.selectedKeys()); } @@ -547,15 +546,12 @@ public final class NioEventLoop extends SingleThreadEventLoop { } } - private void processSelectedKeysOptimized(SelectionKey[] selectedKeys) { - for (int i = 0;; i ++) { - final SelectionKey k = selectedKeys[i]; - if (k == null) { - break; - } + private void processSelectedKeysOptimized() { + for (int i = 0; i < selectedKeys.size; ++i) { + final SelectionKey k = selectedKeys.keys[i]; // null out entry in the array to allow to have it GC'ed once the Channel close // See https://github.com/netty/netty/issues/2363 - selectedKeys[i] = null; + selectedKeys.keys[i] = null; final Object a = k.attachment(); @@ -570,21 +566,9 @@ public final class NioEventLoop extends SingleThreadEventLoop { if (needsToSelectAgain) { // null out entries in the array to allow to have it GC'ed once the Channel close // See https://github.com/netty/netty/issues/2363 - for (;;) { - i++; - if (selectedKeys[i] == null) { - break; - } - selectedKeys[i] = null; - } + selectedKeys.reset(i + 1); selectAgain(); - // Need to flip the optimized selectedKeys to get the right reference to the array - // and reset the index to -1 which will then set to 0 on the for loop - // to start over again. - // - // See https://github.com/netty/netty/issues/1523 - selectedKeys = this.selectedKeys.flip(); i = -1; } } @@ -704,6 +688,10 @@ public final class NioEventLoop extends SingleThreadEventLoop { } } + Selector unwrappedSelector() { + return unwrappedSelector; + } + int selectNow() throws IOException { try { return selector.selectNow(); diff --git a/transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySet.java b/transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySet.java index 36ed9dafa0..534e27f6c7 100644 --- a/transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySet.java +++ b/transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySet.java @@ -13,24 +13,20 @@ * License for the specific language governing permissions and limitations * under the License. */ - package io.netty.channel.nio; import java.nio.channels.SelectionKey; import java.util.AbstractSet; +import java.util.Arrays; import java.util.Iterator; final class SelectedSelectionKeySet extends AbstractSet { - private SelectionKey[] keysA; - private int keysASize; - private SelectionKey[] keysB; - private int keysBSize; - private boolean isA = true; + SelectionKey[] keys; + int size; SelectedSelectionKeySet() { - keysA = new SelectionKey[1024]; - keysB = keysA.clone(); + keys = new SelectionKey[1024]; } @Override @@ -39,58 +35,17 @@ final class SelectedSelectionKeySet extends AbstractSet { return false; } - if (isA) { - int size = keysASize; - keysA[size ++] = o; - keysASize = size; - if (size == keysA.length) { - doubleCapacityA(); - } - } else { - int size = keysBSize; - keysB[size ++] = o; - keysBSize = size; - if (size == keysB.length) { - doubleCapacityB(); - } + keys[size++] = o; + if (size == keys.length) { + increaseCapacity(); } return true; } - private void doubleCapacityA() { - SelectionKey[] newKeysA = new SelectionKey[keysA.length << 1]; - System.arraycopy(keysA, 0, newKeysA, 0, keysASize); - keysA = newKeysA; - } - - private void doubleCapacityB() { - SelectionKey[] newKeysB = new SelectionKey[keysB.length << 1]; - System.arraycopy(keysB, 0, newKeysB, 0, keysBSize); - keysB = newKeysB; - } - - SelectionKey[] flip() { - if (isA) { - isA = false; - keysA[keysASize] = null; - keysBSize = 0; - return keysA; - } else { - isA = true; - keysB[keysBSize] = null; - keysASize = 0; - return keysB; - } - } - @Override public int size() { - if (isA) { - return keysASize; - } else { - return keysBSize; - } + return size; } @Override @@ -107,4 +62,19 @@ final class SelectedSelectionKeySet extends AbstractSet { public Iterator iterator() { throw new UnsupportedOperationException(); } + + void reset() { + reset(0); + } + + void reset(int start) { + Arrays.fill(keys, start, size, null); + size = 0; + } + + private void increaseCapacity() { + SelectionKey[] newKeys = new SelectionKey[keys.length << 1]; + System.arraycopy(keys, 0, newKeys, 0, size); + keys = newKeys; + } } diff --git a/transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySetSelector.java b/transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySetSelector.java new file mode 100644 index 0000000000..998351292f --- /dev/null +++ b/transport/src/main/java/io/netty/channel/nio/SelectedSelectionKeySetSelector.java @@ -0,0 +1,80 @@ +/* + * Copyright 2017 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.channel.nio; + +import java.io.IOException; +import java.nio.channels.SelectionKey; +import java.nio.channels.Selector; +import java.nio.channels.spi.SelectorProvider; +import java.util.Set; + +final class SelectedSelectionKeySetSelector extends Selector { + private final SelectedSelectionKeySet selectionKeys; + private final Selector delegate; + + SelectedSelectionKeySetSelector(Selector delegate, SelectedSelectionKeySet selectionKeys) { + this.delegate = delegate; + this.selectionKeys = selectionKeys; + } + + @Override + public boolean isOpen() { + return delegate.isOpen(); + } + + @Override + public SelectorProvider provider() { + return delegate.provider(); + } + + @Override + public Set keys() { + return delegate.keys(); + } + + @Override + public Set selectedKeys() { + return delegate.selectedKeys(); + } + + @Override + public int selectNow() throws IOException { + selectionKeys.reset(); + return delegate.selectNow(); + } + + @Override + public int select(long timeout) throws IOException { + selectionKeys.reset(); + return delegate.select(timeout); + } + + @Override + public int select() throws IOException { + selectionKeys.reset(); + return delegate.select(); + } + + @Override + public Selector wakeup() { + return delegate.wakeup(); + } + + @Override + public void close() throws IOException { + delegate.close(); + } +} diff --git a/transport/src/test/java/io/netty/channel/nio/SelectedSelectionKeySetTest.java b/transport/src/test/java/io/netty/channel/nio/SelectedSelectionKeySetTest.java new file mode 100644 index 0000000000..5c32001b8e --- /dev/null +++ b/transport/src/test/java/io/netty/channel/nio/SelectedSelectionKeySetTest.java @@ -0,0 +1,66 @@ +/* + * Copyright 2017 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.channel.nio; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import java.nio.channels.SelectionKey; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +public class SelectedSelectionKeySetTest { + @Mock + private SelectionKey mockKey; + @Mock + private SelectionKey mockKey2; + + @Before + public void setup() { + MockitoAnnotations.initMocks(this); + } + + @Test + public void addElements() { + SelectedSelectionKeySet set = new SelectedSelectionKeySet(); + final int expectedSize = 1000000; + for (int i = 0; i < expectedSize; ++i) { + assertTrue(set.add(mockKey)); + } + + assertEquals(expectedSize, set.size()); + assertFalse(set.isEmpty()); + } + + @Test + public void resetSet() { + SelectedSelectionKeySet set = new SelectedSelectionKeySet(); + assertTrue(set.add(mockKey)); + assertTrue(set.add(mockKey2)); + set.reset(1); + + assertSame(mockKey, set.keys[0]); + assertNull(set.keys[1]); + assertEquals(0, set.size()); + assertTrue(set.isEmpty()); + } +}