Do not retain HashMap$KeySet()

This adds a defensive copy of the key set so we do not end up retaining
the original map via a view.

This patch also ensures that (Immutable)OffsetMap retain iteration order
across cache hits.

Change-Id: I1dfa8441093e289a207bd7a11b0a9299e2161d63
Signed-off-by: Robert Varga <rovarga@cisco.com>

diff --git a/common/util/src/main/java/org/opendaylight/yangtools/util/ImmutableOffsetMap.java b/common/util/src/main/java/org/opendaylight/yangtools/util/ImmutableOffsetMap.java
index 1366856c773093046c0e4cd8e30ea9436213896e..02d8fe4d4ac0c7f5f2a123352ffc2a4fb7979d52 100644 (file)
--- a/common/util/src/main/java/org/opendaylight/yangtools/util/ImmutableOffsetMap.java
+++ b/common/util/src/main/java/org/opendaylight/yangtools/util/ImmutableOffsetMap.java
@@ -94,12 +94,13 @@ public final class ImmutableOffsetMap<K, V> implements UnmodifiableMapPhase<K, V
             return ImmutableMap.of();
         }
         if (size == 1) {
-            // Efficient single-entry implementation
+            // Efficient single-entry implementation.
             final Entry<K, V> e = m.entrySet().iterator().next();
             return SharedSingletonMap.of(e.getKey(), e.getValue());
         }
 
-        final Map<K, Integer> offsets = OffsetMapCache.offsetsFor(m.keySet());
+        // copyOf() disconnects the key set while retaining its order across cache lookup.
+        final Map<K, Integer> offsets = OffsetMapCache.orderedOffsets(m.keySet());
         @SuppressWarnings("unchecked")
         final V[] array = (V[]) new Object[offsets.size()];
         for (Entry<K, V> e : m.entrySet()) {
@@ -235,7 +236,7 @@ public final class ImmutableOffsetMap<K, V> implements UnmodifiableMapPhase<K, V
 
     @Override
     public MutableOffsetMap<K, V> toModifiableMap() {
-        return new MutableOffsetMap<>(this);
+        return MutableOffsetMap.copyOf(this);
     }
 
     @Override
@@ -324,6 +325,7 @@ public final class ImmutableOffsetMap<K, V> implements UnmodifiableMapPhase<K, V
     private void readObject(final ObjectInputStream in) throws IOException, ClassNotFoundException {
         final int s = in.readInt();
 
+        // ImmutableList.build() does not have a sizing hint ...
         final List<K> keys = new ArrayList<>(s);
         final V[] values = (V[]) new Object[s];
 
@@ -332,7 +334,7 @@ public final class ImmutableOffsetMap<K, V> implements UnmodifiableMapPhase<K, V
             values[i] = (V)in.readObject();
         }
 
-        setField(OFFSETS_FIELD, OffsetMapCache.offsetsFor(keys));
+        setField(OFFSETS_FIELD, OffsetMapCache.orderedOffsets(keys));
         setField(ARRAY_FIELD, values);
     }
 }

diff --git a/common/util/src/main/java/org/opendaylight/yangtools/util/MutableOffsetMap.java b/common/util/src/main/java/org/opendaylight/yangtools/util/MutableOffsetMap.java
index 014a59f1c3f2172a2936bab41659676ed76bf27e..dfb99d23d16560eeea38ede172ade85ee04974c5 100644 (file)
--- a/common/util/src/main/java/org/opendaylight/yangtools/util/MutableOffsetMap.java
+++ b/common/util/src/main/java/org/opendaylight/yangtools/util/MutableOffsetMap.java
@@ -10,16 +10,16 @@ package org.opendaylight.yangtools.util;
 import com.google.common.annotations.Beta;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import java.util.AbstractMap;
 import java.util.AbstractSet;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
 import java.util.ConcurrentModificationException;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Set;
@@ -50,14 +50,15 @@ public final class MutableOffsetMap<K, V> extends AbstractMap<K, V> implements C
     private boolean needClone = true;
 
     public MutableOffsetMap() {
-        this(Collections.<K>emptySet());
+        this(ImmutableList.<K>of());
     }
 
+    @VisibleForTesting
     @SuppressWarnings("unchecked")
-    protected MutableOffsetMap(final Collection<K> keySet) {
+    MutableOffsetMap(final List<K> keySet) {
         if (!keySet.isEmpty()) {
             removed = keySet.size();
-            offsets = OffsetMapCache.offsetsFor(keySet);
+            offsets = OffsetMapCache.orderedOffsets(keySet);
             objects = (V[])new Object[removed];
         } else {
             offsets = ImmutableMap.of();
@@ -67,15 +68,6 @@ public final class MutableOffsetMap<K, V> extends AbstractMap<K, V> implements C
         this.newKeys = new LinkedHashMap<>();
     }
 
-    protected MutableOffsetMap(final ImmutableOffsetMap<K, V> m) {
-        this(m.offsets(), m.objects());
-    }
-
-    @SuppressWarnings("unchecked")
-    protected MutableOffsetMap(final Map<K, V> m) {
-        this(OffsetMapCache.offsetsFor(m.keySet()), (V[])m.values().toArray());
-    }
-
     protected MutableOffsetMap(final MutableOffsetMap<K, V> m) {
         this.offsets = m.offsets;
         this.objects = m.objects;
@@ -94,10 +86,13 @@ public final class MutableOffsetMap<K, V> extends AbstractMap<K, V> implements C
             return ((MutableOffsetMap<K, V>) m).clone();
         }
         if (m instanceof ImmutableOffsetMap) {
-            return ((ImmutableOffsetMap<K, V>) m).toModifiableMap();
+            final ImmutableOffsetMap<K, V> map = (ImmutableOffsetMap<K, V>) m;
+            return new MutableOffsetMap<>(map.offsets(), map.objects());
         }
 
-        return new MutableOffsetMap<>(m);
+        @SuppressWarnings("unchecked")
+        final V[] values = (V[])m.values().toArray();
+        return new MutableOffsetMap<>(OffsetMapCache.orderedOffsets(m.keySet()), values);
     }
 
     public static <K, V> MutableOffsetMap<K, V> forOffsets(final Map<K, Integer> offsets) {
@@ -106,10 +101,6 @@ public final class MutableOffsetMap<K, V> extends AbstractMap<K, V> implements C
         return new MutableOffsetMap<>(offsets, objects);
     }
 
-    public static <K, V> MutableOffsetMap<K, V> forKeySet(final Collection<K> keySet) {
-        return forOffsets(OffsetMapCache.offsetsFor(keySet));
-    }
-
     @Override
     public int size() {
         return offsets.size() + newKeys.size() - removed;
@@ -223,7 +214,7 @@ public final class MutableOffsetMap<K, V> extends AbstractMap<K, V> implements C
         }
 
         // Construct the set of keys
-        final Collection<K> keyset = new ArrayList<>(s);
+        final List<K> keyset = new ArrayList<>(s);
         if (removed != 0) {
             if (removed != offsets.size()) {
                 for (Entry<K, Integer> e : offsets.entrySet()) {
@@ -258,7 +249,7 @@ public final class MutableOffsetMap<K, V> extends AbstractMap<K, V> implements C
             values[i++] = v;
         }
 
-        return new ImmutableOffsetMap<>(OffsetMapCache.offsetsFor(keyset), values);
+        return new ImmutableOffsetMap<>(OffsetMapCache.orderedOffsets(keyset), values);
     }
 
     @Override

diff --git a/common/util/src/main/java/org/opendaylight/yangtools/util/OffsetMapCache.java b/common/util/src/main/java/org/opendaylight/yangtools/util/OffsetMapCache.java
index 0863e286f6c37be1cc4dc9d66261a9623a38f3dc..0c19c544e635a3849e902695fcce9ebc5453b1ae 100644 (file)
--- a/common/util/src/main/java/org/opendaylight/yangtools/util/OffsetMapCache.java
+++ b/common/util/src/main/java/org/opendaylight/yangtools/util/OffsetMapCache.java
@@ -10,8 +10,10 @@ package org.opendaylight.yangtools.util;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMap.Builder;
+import com.google.common.collect.ImmutableSet;
 import java.util.Collection;
 import java.util.Map;
 
@@ -35,8 +37,19 @@ final class OffsetMapCache {
         throw new UnsupportedOperationException();
     }
 
+    /**
+     * Ordered lookup of offsets. Returned map will have the same iteration order.
+     */
     @SuppressWarnings("unchecked")
-    static <T> Map<T, Integer> offsetsFor(final Collection<T> args) {
-        return (Map<T, Integer>) CACHE.getUnchecked(args);
+    static <T> Map<T, Integer> orderedOffsets(final Collection<T> args) {
+        return (Map<T, Integer>) CACHE.getUnchecked(ImmutableList.copyOf(args));
+    }
+
+    /**
+     * Unordered lookup of offsets. Returned map can have a different iterator order.
+     */
+    @SuppressWarnings("unchecked")
+    static <T> Map<T, Integer> unorderedOffsets(final Collection<T> args) {
+        return (Map<T, Integer>) CACHE.getUnchecked(ImmutableSet.copyOf(args));
     }
 }

diff --git a/common/util/src/main/java/org/opendaylight/yangtools/util/SharedSingletonMap.java b/common/util/src/main/java/org/opendaylight/yangtools/util/SharedSingletonMap.java
index 17ff0d985ad7b2c903cf8b2379df27a1e45f6a3d..4c285de0fd4005e07c973497a7e6208c30e64f9d 100644 (file)
--- a/common/util/src/main/java/org/opendaylight/yangtools/util/SharedSingletonMap.java
+++ b/common/util/src/main/java/org/opendaylight/yangtools/util/SharedSingletonMap.java
@@ -56,7 +56,7 @@ public final class SharedSingletonMap<K, V> implements Serializable, Unmodifiabl
 
     @Override
     public ModifiableMapPhase<K, V> toModifiableMap() {
-        return new MutableOffsetMap<K, V>(this);
+        return MutableOffsetMap.copyOf(this);
     }
 
     @Override

diff --git a/common/util/src/test/java/org/opendaylight/yangtools/util/OffsetMapTest.java b/common/util/src/test/java/org/opendaylight/yangtools/util/OffsetMapTest.java
index 289c133842a522fb829a8f3f5c1d82eb56474069..0406172e57117f3ebf2af9c02c97ed9d1109a00c 100644 (file)
--- a/common/util/src/test/java/org/opendaylight/yangtools/util/OffsetMapTest.java
+++ b/common/util/src/test/java/org/opendaylight/yangtools/util/OffsetMapTest.java
@@ -14,8 +14,8 @@ import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterators;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -352,7 +352,7 @@ public class OffsetMapTest {
 
     @Test
     public void testMutableWithKeyset() {
-        final MutableOffsetMap<String, String> map = new MutableOffsetMap<>(ImmutableSet.of("k1", "k2"));
+        final MutableOffsetMap<String, String> map = new MutableOffsetMap<>(ImmutableList.of("k1", "k2"));
         assertTrue(map.isEmpty());
         assertTrue(map.keySet().isEmpty());
         assertNull(map.get("k1"));