This is an automated email from the ASF dual-hosted git repository.
onichols pushed a commit to branch support/1.12
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/support/1.12 by this push:
new 5bac21f GEODE-8564: Updated CopyOnWriteHashSet's iterator
implementation to (#5583)
5bac21f is described below
commit 5bac21fd431a9a89f6f58887a2fa6cb714cc238b
Author: Udo Kohlmeyer <[email protected]>
AuthorDate: Thu Oct 1 14:22:21 2020 +1000
GEODE-8564: Updated CopyOnWriteHashSet's iterator implementation to (#5583)
support iterator.remove().
Updated JCAManagedConnection to not use iterator.remove(). It will now
collect all items to remove and then use the CopyOnWriteHashSet removeAll
functionality to avoid unnecessary intermediary collection creations.
(cherry picked from commit 66bcce8bb27d1fae4a115517ef39cadcd2189aa8)
---
.../internal/ra/spi/JCAManagedConnection.java | 57 +++++++++++-----------
.../apache/geode/internal/CopyOnWriteHashSet.java | 32 ++++++++++--
.../internal/CopyOnWriteHashSetJUnitTest.java | 20 ++++++++
3 files changed, 78 insertions(+), 31 deletions(-)
diff --git
a/geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java
b/geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java
index 575b2a2..44af47c 100644
---
a/geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java
+++
b/geode-core/src/jca/java/org/apache/geode/internal/ra/spi/JCAManagedConnection.java
@@ -16,7 +16,8 @@
package org.apache.geode.internal.ra.spi;
import java.io.PrintWriter;
-import java.util.Iterator;
+import java.util.ArrayList;
+import java.util.LinkedList;
import java.util.List;
import java.util.Set;
import java.util.concurrent.CopyOnWriteArrayList;
@@ -53,9 +54,9 @@ public class JCAManagedConnection implements
ManagedConnection {
private final JCAManagedConnectionFactory connectionFactory;
- private final Set<GFConnectionImpl> connections = new
CopyOnWriteHashSet<>();;
+ private final Set<GFConnectionImpl> connections = new CopyOnWriteHashSet<>();
- private volatile JCALocalTransaction localTransaction = new
JCALocalTransaction();;
+ private volatile JCALocalTransaction localTransaction = new
JCALocalTransaction();
JCAManagedConnection(JCAManagedConnectionFactory connectionFactory) {
this.connectionFactory = connectionFactory;
@@ -78,14 +79,7 @@ public class JCAManagedConnection implements
ManagedConnection {
@Override
public void cleanup() throws ResourceException {
- synchronized (this.connections) {
- Iterator<GFConnectionImpl> iterator = this.connections.iterator();
- while (iterator.hasNext()) {
- GFConnectionImpl connection = iterator.next();
- connection.invalidate();
- iterator.remove();
- }
- }
+ invalidateAndRemoveConnections();
if (this.localTransaction == null ||
this.localTransaction.transactionInProgress()) {
if (this.initialized && !isCacheClosed()) {
this.localTransaction = new JCALocalTransaction(this.cache,
this.transactionManager);
@@ -95,6 +89,23 @@ public class JCAManagedConnection implements
ManagedConnection {
}
}
+ /**
+ * Invalidate and remove the {@link GFConnectionImpl} from the connections
collection.
+ * The approach to use removeAll instead of Iterator.remove is purely a
performance optimization
+ * to avoid creating all the intermediary collections that will be created
when using the single
+ * remove operation.
+ */
+ private void invalidateAndRemoveConnections() {
+ synchronized (this.connections) {
+ List<GFConnectionImpl> connectionsToRemove = new ArrayList<>();
+ for (GFConnectionImpl connection : this.connections) {
+ connection.invalidate();
+ connectionsToRemove.add(connection);
+ }
+ connections.removeAll(connectionsToRemove);
+ }
+ }
+
private boolean isCacheClosed() {
if (this.cache != null) {
return this.cache.isClosed();
@@ -104,14 +115,7 @@ public class JCAManagedConnection implements
ManagedConnection {
@Override
public void destroy() throws ResourceException {
- synchronized (this.connections) {
- Iterator<GFConnectionImpl> iterator = this.connections.iterator();
- while (iterator.hasNext()) {
- GFConnectionImpl connection = iterator.next();
- connection.invalidate();
- iterator.remove();
- }
- }
+ invalidateAndRemoveConnections();
this.transactionManager = null;
this.cache = null;
this.localTransaction = null;
@@ -188,11 +192,10 @@ public class JCAManagedConnection implements
ManagedConnection {
this.localTransaction = null;
synchronized (this.connections) {
- Iterator<GFConnectionImpl> iterator = this.connections.iterator();
- while (iterator.hasNext()) {
- GFConnectionImpl connection = iterator.next();
+ List<GFConnectionImpl> connectionsToRemove = new
LinkedList<>(connections);
+ for (GFConnectionImpl connection : connections) {
connection.invalidate();
-
+ connectionsToRemove.add(connection);
synchronized (this.listeners) {
ConnectionEvent event =
new ConnectionEvent(this,
ConnectionEvent.CONNECTION_ERROR_OCCURRED, e);
@@ -201,9 +204,8 @@ public class JCAManagedConnection implements
ManagedConnection {
listener.connectionErrorOccurred(event);
}
}
-
- iterator.remove();
}
+ connections.removeAll(connectionsToRemove);
}
}
@@ -212,11 +214,10 @@ public class JCAManagedConnection implements
ManagedConnection {
this.connections.remove(connection);
synchronized (this.listeners) {
- Iterator<ConnectionEventListener> iterator = this.listeners.iterator();
ConnectionEvent event = new ConnectionEvent(this,
ConnectionEvent.CONNECTION_CLOSED);
event.setConnectionHandle(connection);
- while (iterator.hasNext()) {
- iterator.next().connectionClosed(event);
+ for (ConnectionEventListener listener : this.listeners) {
+ listener.connectionClosed(event);
}
}
diff --git
a/geode-core/src/main/java/org/apache/geode/internal/CopyOnWriteHashSet.java
b/geode-core/src/main/java/org/apache/geode/internal/CopyOnWriteHashSet.java
index 119aaa5..fd85079 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/CopyOnWriteHashSet.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/CopyOnWriteHashSet.java
@@ -22,7 +22,9 @@ import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
+import java.util.LinkedList;
import java.util.Set;
+import java.util.function.Consumer;
/**
* A Hash set where every modification makes an internal copy of a HashSet.
Similar to
@@ -47,12 +49,36 @@ public class CopyOnWriteHashSet<T> implements Set<T>,
Serializable {
}
/**
- * Because I'm lazy, this iterator does not support modification of this
set. If you need it, it
- * shouldn't be too hard to implement.
+ * Create a custom {@link Iterator} implementation for the {@link
CopyOnWriteHashSet}
*/
@Override
public Iterator<T> iterator() {
- return Collections.unmodifiableSet(snapshot).iterator();
+ return new Iterator<T>() {
+
+ private Iterator<T> iterator = new LinkedList<>(snapshot).iterator();
+ private T currentElement;
+
+ @Override
+ public boolean hasNext() {
+ return iterator.hasNext();
+ }
+
+ @Override
+ public T next() {
+ currentElement = iterator.next();
+ return currentElement;
+ }
+
+ @Override
+ public void remove() {
+ snapshot.remove(currentElement);
+ }
+
+ @Override
+ public void forEachRemaining(Consumer<? super T> action) {
+ iterator.forEachRemaining(action);
+ }
+ };
}
@Override
diff --git
a/geode-core/src/test/java/org/apache/geode/internal/CopyOnWriteHashSetJUnitTest.java
b/geode-core/src/test/java/org/apache/geode/internal/CopyOnWriteHashSetJUnitTest.java
index f7fa32b..de3e030 100644
---
a/geode-core/src/test/java/org/apache/geode/internal/CopyOnWriteHashSetJUnitTest.java
+++
b/geode-core/src/test/java/org/apache/geode/internal/CopyOnWriteHashSetJUnitTest.java
@@ -27,6 +27,7 @@ import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;
+import org.assertj.core.api.Assertions;
import org.junit.Test;
@@ -44,6 +45,25 @@ public class CopyOnWriteHashSetJUnitTest {
}
@Test
+ public void testIteratorRemove() {
+ CopyOnWriteHashSet<String> startingCollection = new
CopyOnWriteHashSet<String>();
+ startingCollection.addAll(Arrays.asList("a", "b", "c", "d"));
+
+ Iterator<String> iterator = startingCollection.iterator();
+ while (iterator.hasNext()) {
+ String element = iterator.next();
+
+ if (element.equals("b")) {
+ iterator.remove();
+ }
+ }
+
+ assertEquals(3, startingCollection.size());
+
+ Assertions.assertThat(startingCollection).containsExactly("a", "c", "d");
+ }
+
+ @Test
public void testAllMethods() throws Exception {
CopyOnWriteHashSet<String> set = new CopyOnWriteHashSet<String>();
assertTrue(set.add("a"));
