You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by on...@apache.org on 2020/10/01 04:23:18 UTC
[geode] branch develop updated: GEODE-8564: Updated
CopyOnWriteHashSet's iterator implementation to (#5583)
This is an automated email from the ASF dual-hosted git repository.
onichols pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 66bcce8 GEODE-8564: Updated CopyOnWriteHashSet's iterator implementation to (#5583)
66bcce8 is described below
commit 66bcce8bb27d1fae4a115517ef39cadcd2189aa8
Author: Udo Kohlmeyer <ko...@users.noreply.github.com>
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.
---
.../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"));