COLLECTIONS-427 patch applied.

git-svn-id: https://svn.apache.org/repos/asf/commons/proper/collections/trunk@1377485 13f79535-47bb-0310-9956-ffa450edef68

src/changes/changes.xml

@@ -75,6 +75,9 @@
     <action issue="COLLECTIONS-405" dev="brentworden" type="add" due-to="Adam Dyga">
       Added "ListUtils#select" and "ListUtils#selectRejected" methods.
     </action>
+    <action issue="COLLECTIONS-427" dev="brentworden" type="fix" due-to="Mert Guldur">
+      Fixed performance issue in "SetUniqueList#retainAll" method for large collections.
+    </action>
   </release>
   </body>
 </document>

src/main/java/org/apache/commons/collections/list/SetUniqueList.java

@@ -29,14 +29,14 @@ import org.apache.commons.collections.iterators.AbstractListIteratorDecorator;
 import org.apache.commons.collections.set.UnmodifiableSet;
 
 /**
- * Decorates a <code>List</code> to ensure that no duplicates are present
+ * Decorates a <code>List</code> to ensure that no duplicates are present much
- * much like a <code>Set</code>.
+ * like a <code>Set</code>.
  * <p>
- * The <code>List</code> interface makes certain assumptions/requirements.
+ * The <code>List</code> interface makes certain assumptions/requirements. This
- * This implementation breaks these in certain ways, but this is merely the
+ * implementation breaks these in certain ways, but this is merely the result of
- * result of rejecting duplicates.
+ * rejecting duplicates. Each violation is explained in the method, but it
- * Each violation is explained in the method, but it should not affect you.
+ * should not affect you. Bear in mind that Sets require immutable objects to
- * Bear in mind that Sets require immutable objects to function correctly.
+ * function correctly.
  * <p>
  * The {@link org.apache.commons.collections.set.ListOrderedSet ListOrderedSet}
  * class provides an alternative approach, by wrapping an existing Set and
@@ -58,15 +58,19 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 	protected final Set<E> set;
 
 	/**
-     * Factory method to create a SetList using the supplied list to retain order.
+	 * Factory method to create a SetList using the supplied list to retain
+	 * order.
 	 * <p>
-     * If the list contains duplicates, these are removed (first indexed one kept).
+	 * If the list contains duplicates, these are removed (first indexed one
-     * A <code>HashSet</code> is used for the set behaviour.
+	 * kept). A <code>HashSet</code> is used for the set behaviour.
 	 * 
-     * @param <E> the element type
+	 * @param <E>
-     * @param list the list to decorate, must not be null
+	 *            the element type
+	 * @param list
+	 *            the list to decorate, must not be null
 	 * @return a new {@link SetUniqueList}
-     * @throws IllegalArgumentException if list is null
+	 * @throws IllegalArgumentException
+	 *             if list is null
 	 */
 	public static <E> SetUniqueList<E> setUniqueList(List<E> list) {
 		if (list == null) {
@@ -84,13 +88,17 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 
 	// -----------------------------------------------------------------------
 	/**
-     * Constructor that wraps (not copies) the List and specifies the set to use.
+	 * Constructor that wraps (not copies) the List and specifies the set to
+	 * use.
 	 * <p>
 	 * The set and list must both be correctly initialised to the same elements.
 	 * 
-     * @param set  the set to decorate, must not be null
+	 * @param set
-     * @param list  the list to decorate, must not be null
+	 *            the set to decorate, must not be null
-     * @throws IllegalArgumentException if set or list is null
+	 * @param list
+	 *            the list to decorate, must not be null
+	 * @throws IllegalArgumentException
+	 *             if set or list is null
 	 */
 	protected SetUniqueList(List<E> list, Set<E> set) {
 		super(list);
@@ -114,12 +122,12 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 	/**
 	 * Adds an element to the list if it is not already present.
 	 * <p>
-     * <i>(Violation)</i>
+	 * <i>(Violation)</i> The <code>List</code> interface requires that this
-     * The <code>List</code> interface requires that this method returns
+	 * method returns <code>true</code> always. However this class may return
-     * <code>true</code> always. However this class may return <code>false</code>
+	 * <code>false</code> because of the <code>Set</code> behaviour.
-     * because of the <code>Set</code> behaviour.
 	 * 
-     * @param object the object to add
+	 * @param object
+	 *            the object to add
 	 * @return true if object was added
 	 */
 	@Override
@@ -135,14 +143,17 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 	}
 
 	/**
-     * Adds an element to a specific index in the list if it is not already present.
+	 * Adds an element to a specific index in the list if it is not already
+	 * present.
 	 * <p>
-     * <i>(Violation)</i>
+	 * <i>(Violation)</i> The <code>List</code> interface makes the assumption
-     * The <code>List</code> interface makes the assumption that the element is
+	 * that the element is always inserted. This may not happen with this
-     * always inserted. This may not happen with this implementation.
+	 * implementation.
 	 * 
-     * @param index  the index to insert at
+	 * @param index
-     * @param object  the object to add
+	 *            the index to insert at
+	 * @param object
+	 *            the object to add
 	 */
 	@Override
 	public void add(int index, E object) {
@@ -159,11 +170,12 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 	 * Only elements that are not already in this list will be added, and
 	 * duplicates from the specified collection will be ignored.
 	 * <p>
-     * <i>(Violation)</i>
+	 * <i>(Violation)</i> The <code>List</code> interface makes the assumption
-     * The <code>List</code> interface makes the assumption that the elements
+	 * that the elements are always inserted. This may not happen with this
-     * are always inserted. This may not happen with this implementation.
+	 * implementation.
 	 * 
-     * @param coll  the collection to add in iterator order
+	 * @param coll
+	 *            the collection to add in iterator order
 	 * @return true if this collection changed
 	 */
 	@Override
@@ -178,12 +190,14 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 	 * Only elements that are not already in this list will be added, and
 	 * duplicates from the specified collection will be ignored.
 	 * <p>
-     * <i>(Violation)</i>
+	 * <i>(Violation)</i> The <code>List</code> interface makes the assumption
-     * The <code>List</code> interface makes the assumption that the elements
+	 * that the elements are always inserted. This may not happen with this
-     * are always inserted. This may not happen with this implementation.
+	 * implementation.
 	 * 
-     * @param index  the index to insert at
+	 * @param index
-     * @param coll  the collection to add in iterator order
+	 *            the index to insert at
+	 * @param coll
+	 *            the collection to add in iterator order
 	 * @return true if this collection changed
 	 */
 	@Override
@@ -201,13 +215,14 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 	/**
 	 * Sets the value at the specified index avoiding duplicates.
 	 * <p>
-     * The object is set into the specified index.
+	 * The object is set into the specified index. Afterwards, any previous
-     * Afterwards, any previous duplicate is removed
+	 * duplicate is removed If the object is not already in the list then a
-     * If the object is not already in the list then a normal set occurs.
+	 * normal set occurs. If it is present, then the old version is removed.
-     * If it is present, then the old version is removed.
 	 * 
-     * @param index  the index to insert at
+	 * @param index
-     * @param object  the object to set
+	 *            the index to insert at
+	 * @param object
+	 *            the object to set
 	 * @return the previous object
 	 */
 	@Override
@@ -254,9 +269,26 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 
 	@Override
 	public boolean retainAll(Collection<?> coll) {
-        boolean result = super.retainAll(coll);
+		Set<Object> setRetainAll = new HashSet<Object>();
-        set.retainAll(coll);
+		for (Iterator<?> it = coll.iterator(); it.hasNext();) {
-        return result;
+			Object next = it.next();
+			if (set.contains(next)) {
+				setRetainAll.add(next);
+			}
+		}
+		if (setRetainAll.size() == set.size()) {
+			return false;
+		}
+		if (setRetainAll.size() == 0) {
+			clear();
+		} else {
+			for (Iterator<E> it = iterator(); it.hasNext();) {
+				if (!setRetainAll.contains(it.next())) {
+					it.remove();
+				}
+			}
+		}
+		return true;
 	}
 
 	@Override
@@ -301,9 +333,12 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 	 * Create a new {@link Set} with the same type as the provided {@code set}
 	 * and populate it with all elements of {@code list}.
 	 * 
-     * @param set the {@link Set} to be used as return type, must not be null
+	 * @param set
-     * @param list the {@link List} to populate the {@link Set}
+	 *            the {@link Set} to be used as return type, must not be null
-     * @return a new {@link Set} populated with all elements of the provided {@link List}
+	 * @param list
+	 *            the {@link List} to populate the {@link Set}
+	 * @return a new {@link Set} populated with all elements of the provided
+	 *         {@link List}
 	 */
 	@SuppressWarnings("unchecked")
 	protected Set<E> createSetBasedOnList(Set<E> set, List<E> list) {
@@ -354,7 +389,8 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 	/**
 	 * Inner class iterator.
 	 */
-    static class SetListListIterator<E> extends AbstractListIteratorDecorator<E> {
+	static class SetListListIterator<E> extends
+			AbstractListIteratorDecorator<E> {
 
 		protected final Set<E> set;
 		protected E last = null;
@@ -393,7 +429,8 @@ public class SetUniqueList<E> extends AbstractSerializableListDecorator<E> {
 
 		@Override
 		public void set(E object) {
-            throw new UnsupportedOperationException("ListIterator does not support set");
+			throw new UnsupportedOperationException(
+					"ListIterator does not support set");
 		}
 	}
 

src/test/java/org/apache/commons/collections/list/SetUniqueListTest.java

@@ -523,6 +523,79 @@ public class SetUniqueListTest<E> extends AbstractListTest<E> {
         assertFalse(subUniqueList.contains("World")); // fails
     }
 
+    @SuppressWarnings("unchecked")
+	public void testRetainAll() {
+    	List<E> list = new ArrayList<E>(10);
+    	SetUniqueList<E> uniqueList = SetUniqueList.setUniqueList(list);
+    	for (int i = 0; i < 10; ++i) {
+    		uniqueList.add((E)Integer.valueOf(i));
+    	}
+    	
+    	Collection<E> retained = new ArrayList<E>(5);
+    	for (int i = 0; i < 5; ++i) {
+    		retained.add((E)Integer.valueOf(i * 2));
+    	}
+    	
+    	assertTrue(uniqueList.retainAll(retained));
+    	assertEquals(5, uniqueList.size());
+    	assertTrue(uniqueList.contains(Integer.valueOf(0)));
+    	assertTrue(uniqueList.contains(Integer.valueOf(2)));
+    	assertTrue(uniqueList.contains(Integer.valueOf(4)));
+    	assertTrue(uniqueList.contains(Integer.valueOf(6)));
+    	assertTrue(uniqueList.contains(Integer.valueOf(8)));
+    }
+
+    @SuppressWarnings("unchecked")
+	public void testRetainAllWithInitialList() {
+    	// initialized with empty list
+    	List<E> list = new ArrayList<E>(10);
+    	for (int i = 0; i < 5; ++i) {
+    		list.add((E)Integer.valueOf(i));
+    	}
+    	SetUniqueList<E> uniqueList = SetUniqueList.setUniqueList(list);
+    	for (int i = 5; i < 10; ++i) {
+    		uniqueList.add((E)Integer.valueOf(i));
+    	}
+    	
+    	Collection<E> retained = new ArrayList<E>(5);
+    	for (int i = 0; i < 5; ++i) {
+    		retained.add((E)Integer.valueOf(i * 2));
+    	}
+    	
+    	assertTrue(uniqueList.retainAll(retained));
+    	assertEquals(5, uniqueList.size());
+    	assertTrue(uniqueList.contains(Integer.valueOf(0)));
+    	assertTrue(uniqueList.contains(Integer.valueOf(2)));
+    	assertTrue(uniqueList.contains(Integer.valueOf(4)));
+    	assertTrue(uniqueList.contains(Integer.valueOf(6)));
+    	assertTrue(uniqueList.contains(Integer.valueOf(8)));
+    }
+    
+    /*
+     * test case for https://issues.apache.org/jira/browse/COLLECTIONS-427
+     */
+    public void testRetainAllCollections427() {
+        int size = 50000;
+        ArrayList<Integer> list = new ArrayList<Integer>();
+        for (int i = 0; i < size; i++) {
+            list.add(i);
+        }
+        SetUniqueList<Integer> uniqueList = SetUniqueList.setUniqueList(list);
+        ArrayList<Integer> toRetain = new ArrayList<Integer>();
+        for (int i = size; i < 2*size; i++) {
+            toRetain.add(i);
+        }
+
+        long start = System.currentTimeMillis();
+        uniqueList.retainAll(toRetain);
+        long stop = System.currentTimeMillis();
+        
+        // make sure retainAll completes under 5 seconds
+        // TODO if test is migrated to JUnit 4, add a Timeout rule.
+        // http://kentbeck.github.com/junit/javadoc/latest/org/junit/rules/Timeout.html
+        assertTrue((stop - start) < 5000);
+    }
+    
     @SuppressWarnings("serial")
     class SetUniqueList307 extends SetUniqueList<E> {
         public SetUniqueList307(List<E> list, Set<E> set) {