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 16:19:17 UTC
[geode] branch support/1.12 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 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 <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.
(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"));