Fixed entrySet, keySet and values to use AbstractSet & AbstractCollection iterators

Author: klockla

service/src/main/java/crawlercommons/urlfrontier/service/AbstractFrontierService.java

@@ -638,7 +638,7 @@ public void getURLs(GetParams request, StreamObserver<URLInfo> responseObserver)
             final QueueInterface currentQueue;
             final QueueWithinCrawl currentCrawlQueue;
 
-            Entry<QueueWithinCrawl, QueueInterface> e = getQueues().firsEntry();
+            Entry<QueueWithinCrawl, QueueInterface> e = getQueues().firstEntry();
             currentQueue = e.getValue();
             currentCrawlQueue = e.getKey();
 

service/src/main/java/crawlercommons/urlfrontier/service/ConcurrentInsertionOrderMap.java

@@ -15,7 +15,7 @@
 public interface ConcurrentInsertionOrderMap<K, V> extends ConcurrentMap<K, V> {
 
     /** Returns the first entry according to insertion order */
-    Entry<K, V> firsEntry();
+    Entry<K, V> firstEntry();
 
     /** Remove & returns the first entry according to insertion order */
     Entry<K, V> pollFirstEntry();

service/src/main/java/crawlercommons/urlfrontier/service/ConcurrentLinkedHashMap.java

@@ -83,7 +83,7 @@ public V put(K key, V value) {
         try {
             previous = map.put(key, value);
             if (previous == null) {
-                order.add(key);
+                order.offer(key);
                 size.incrementAndGet();
             }
         } finally {

service/src/main/java/crawlercommons/urlfrontier/service/ConcurrentLinkedStripedMap.java

@@ -187,7 +187,7 @@ public Set<K> keySet() {
     /** Returns a Collection view of the values contained in this map in insertion order. */
     @Override
     public Collection<V> values() {
-        List<V> values = new ArrayList<V>(this.size.get());
+        List<V> values = new ArrayList<>(this.size.get());
         for (K key : order) {
             V value = map.get(key);
             if (value != null) {

service/src/main/java/crawlercommons/urlfrontier/service/ConcurrentOrderedMap.java

@@ -1,20 +1,17 @@
 package crawlercommons.urlfrontier.service;
 
-
+import java.util.AbstractCollection;
 import java.util.AbstractMap;
-import java.util.AbstractMap.SimpleImmutableEntry;
-import java.util.ArrayList;
+import java.util.AbstractSet;
 import java.util.Collection;
-import java.util.LinkedHashSet;
-import java.util.List;
+import java.util.Iterator;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.locks.StampedLock;
-import java.util.stream.Collectors;
 
 /**
  * Concurrent version of LinkedHashMap Design goal is the same as for ConcurrentHashMap: Maintain
@@ -112,61 +109,77 @@ public V remove(Object key) {
 
     @Override
     /**
-     * Insertion order is preserved. The entry set returned is not backed up by the map.
+     * Insertion order is preserved.
      *
      * @return a linked hash set will all keys
      */
     public Set<K> keySet() {
-        // Return entries in insertion order
-        long stamp = lock.tryOptimisticRead();
-
-        Set<K> orderedKeys = new LinkedHashSet<>(insertionOrderMap.values());
-
-        // Validate the read to ensure no concurrent modifications
-        if (!lock.validate(stamp)) {
-            stamp = lock.readLock();
-            try {
-                orderedKeys = new LinkedHashSet<>(insertionOrderMap.values());
-            } finally {
-                lock.unlockRead(stamp);
+        return new AbstractSet<>() {
+            @Override
+            public Iterator<K> iterator() {
+                return new Iterator<>() {
+                    final Iterator<Entry<Long, K>> orderedIterator =
+                            insertionOrderMap.entrySet().iterator();
+
+                    @Override
+                    public boolean hasNext() {
+                        return orderedIterator.hasNext();
+                    }
+
+                    @Override
+                    public K next() {
+                        return orderedIterator.next().getValue();
+                    }
+
+                    @Override
+                    public void remove() {
+                        throw new UnsupportedOperationException();
+                    }
+                };
             }
-        }
 
-        return orderedKeys;
+            @Override
+            public int size() {
+                // Don't use size on CSLM as it's not in O(1) but rather O(n) and may be inacurate
+                return valueMap.size();
+            }
+        };
     }
 
     /**
-     * Insertion order is preserved. The entry set returned is not backed up by the map.
+     * Insertion order is preserved.
      *
      * @return a linked hash set will all entries
      */
     @Override
-    public Set<Entry<K, V>> entrySet() {
-        // Return entries in insertion order
-        long stamp = lock.tryOptimisticRead();
-
-        Set<Entry<K, V>> orderedEntries =
-                insertionOrderMap.values().stream()
-                        .map(key -> new SimpleImmutableEntry<>(key, valueMap.get(key).value))
-                        .collect(Collectors.toCollection(LinkedHashSet::new));
-
-        // Validate the read to ensure no concurrent modifications
-        if (!lock.validate(stamp)) {
-            stamp = lock.readLock();
-            try {
-                orderedEntries =
-                        insertionOrderMap.values().stream()
-                                .map(
-                                        key ->
-                                                new SimpleImmutableEntry<>(
-                                                        key, valueMap.get(key).value))
-                                .collect(Collectors.toCollection(LinkedHashSet::new));
-            } finally {
-                lock.unlockRead(stamp);
+    public Set<Map.Entry<K, V>> entrySet() {
+        return new AbstractSet<Map.Entry<K, V>>() {
+            @Override
+            public Iterator<Map.Entry<K, V>> iterator() {
+                return new Iterator<Map.Entry<K, V>>() {
+                    final Iterator<Entry<Long, K>> orderedIterator =
+                            insertionOrderMap.entrySet().iterator();
+
+                    @Override
+                    public boolean hasNext() {
+                        return orderedIterator.hasNext();
+                    }
+
+                    @Override
+                    public Map.Entry<K, V> next() {
+                        Entry<Long, K> nextEntry = orderedIterator.next();
+                        K key = nextEntry.getValue();
+                        V value = valueMap.get(key).value;
+                        return new AbstractMap.SimpleImmutableEntry<>(key, value);
+                    }
+                };
             }
-        }
 
-        return orderedEntries;
+            @Override
+            public int size() {
+                return valueMap.size();
+            }
+        };
     }
 
     @Override
@@ -238,14 +251,23 @@ public boolean remove(Object key, Object value) {
     }
 
     @Override
-    // FIXME: Should be atomic but stamped lock is not reentrant
     public V replace(K key, V value) {
 
         long stamp = lock.writeLock();
         try {
             if (valueMap.containsKey(key)) {
-                return put(key, value);
-            } else return null;
+                ValueEntry vEntry = valueMap.get(key);
+                V oldValue = vEntry.value;
+                vEntry.value = value;
+
+                return oldValue;
+            } else {
+                long newOrder = insertionCounter.getAndIncrement();
+                insertionOrderMap.put(newOrder, key);
+                valueMap.put(key, new ValueEntry(value, newOrder));
+
+                return null;
+            }
         } finally {
             lock.unlockWrite(stamp);
         }
@@ -254,7 +276,7 @@ public V replace(K key, V value) {
     /*
      * Returns the first entry according to insertion order
      */
-    public Entry<K, V> firsEntry() {
+    public Entry<K, V> firstEntry() {
         K key = insertionOrderMap.firstEntry().getValue();
 
         return new AbstractMap.SimpleImmutableEntry<>(key, valueMap.get(key).value);
@@ -304,36 +326,54 @@ public boolean containsValue(Object value) {
 
     @Override
     public void putAll(Map<? extends K, ? extends V> m) {
-        // TODO Implement this optional operation
-        throw new UnsupportedOperationException();
+
+        long stamp = lock.writeLock();
+        try {
+            for (Entry<? extends K, ? extends V> entry : m.entrySet()) {
+                K key = entry.getKey();
+
+                // Check if key already exists
+                ValueEntry ventry = valueMap.get(key);
+                if (ventry != null) {
+                    ventry.value = entry.getValue();
+                } else {
+                    long newOrder = insertionCounter.getAndIncrement();
+                    insertionOrderMap.put(newOrder, key);
+                    valueMap.put(key, new ValueEntry(entry.getValue(), newOrder));
+                }
+            }
+        } finally {
+            lock.unlock(stamp);
+        }
     }
 
     /** Returns a Collection view of the values contained in this map in insertion order. */
     @Override
     public Collection<V> values() {
-        List<V> values;
-
-        // Return entries in insertion order
-        long stamp = lock.tryOptimisticRead();
-
-        values =
-                insertionOrderMap.values().stream()
-                        .map(key -> valueMap.get(key).value)
-                        .collect(Collectors.toCollection(ArrayList::new));
-
-        // Validate the read to ensure no concurrent modifications
-        if (!lock.validate(stamp)) {
-            stamp = lock.readLock();
-            try {
-                values =
-                        insertionOrderMap.values().stream()
-                                .map(key -> valueMap.get(key).value)
-                                .collect(Collectors.toCollection(ArrayList::new));
-            } finally {
-                lock.unlockRead(stamp);
+        return new AbstractCollection<>() {
+            @Override
+            public Iterator<V> iterator() {
+                return new Iterator<>() {
+                    final Iterator<Entry<Long, K>> orderedIterator =
+                            insertionOrderMap.entrySet().iterator();
+
+                    @Override
+                    public boolean hasNext() {
+                        return orderedIterator.hasNext();
+                    }
+
+                    @Override
+                    public V next() {
+                        K key = orderedIterator.next().getValue();
+                        return valueMap.get(key).value;
+                    }
+                };
             }
-        }
 
-        return values;
+            @Override
+            public int size() {
+                return valueMap.size();
+            }
+        };
     }
 }