classlib: refactor LinkedHashMap

I found regression somewhere in LinkedHashMap, when certain operations cause cycle in entry list. Since updated logic was totally unclear to me, I rewrote it from scratch. Also, reverted mechanism to remove entries NOT by keys and used it where necessary to improve performance.

classlib/src/main/java/org/teavm/classlib/java/util/THashMap.java

@@ -20,7 +20,11 @@ import java.util.function.BiConsumer;
 import java.util.function.BiFunction;
 import java.util.function.Consumer;
 import org.teavm.classlib.java.io.TSerializable;
-import org.teavm.classlib.java.lang.*;
+import org.teavm.classlib.java.lang.TCloneNotSupportedException;
+import org.teavm.classlib.java.lang.TCloneable;
+import org.teavm.classlib.java.lang.TIllegalArgumentException;
+import org.teavm.classlib.java.lang.TIllegalStateException;
+import org.teavm.classlib.java.lang.TObject;
 import org.teavm.interop.Rename;
 
 public class THashMap<K, V> extends TAbstractMap<K, V> implements TCloneable, TSerializable {
@@ -192,10 +196,10 @@ public class THashMap<K, V> extends TAbstractMap<K, V> implements TCloneable, TS
         @Override
         public boolean remove(Object object) {
             if (object instanceof TMap.Entry) {
-                TMap.Entry<?, ?> oEntry = (TMap.Entry<?, ?>) object;
+                var oEntry = (TMap.Entry<?, ?>) object;
-                TMap.Entry<K, V> entry = associatedMap.entryByKey(oEntry.getKey());
+                var entry = associatedMap.entryByKey(oEntry.getKey());
                 if (entry != null && TObjects.equals(entry.getValue(), oEntry.getValue())) {
-                    associatedMap.removeByKey(entry.getKey());
+                    associatedMap.removeEntry(entry);
                     return true;
                 }
             }
@@ -531,6 +535,21 @@ public class THashMap<K, V> extends TAbstractMap<K, V> implements TCloneable, TS
         return null;
     }
 
+    final void removeEntry(HashEntry<K, V> entry) {
+        int index = entry.origKeyHash & (elementData.length - 1);
+        var m = elementData[index];
+        if (m == entry) {
+            elementData[index] = entry.next;
+        } else {
+            while (m.next != entry) {
+                m = m.next;
+            }
+            m.next = entry.next;
+        }
+        modCount++;
+        elementCount--;
+    }
+
     final HashEntry<K, V> removeByKey(Object key) {
         int index = 0;
         HashEntry<K, V> entry;

classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMap.java

@@ -15,6 +15,7 @@
  */
 package org.teavm.classlib.java.util;
 
+import java.util.Objects;
 import java.util.function.BiConsumer;
 import java.util.function.BiFunction;
 
@@ -63,8 +64,8 @@ public class TLinkedHashMap<K, V> extends THashMap<K, V> implements TSequencedMa
             rehash(capacity);
         }
         for (var it = map.entrySet().iterator(); it.hasNext();) {
-            TMap.Entry<? extends K, ? extends V> entry = it.next();
+            var entry = it.next();
-            putImpl(entry.getKey(), entry.getValue(), false);
+            putImpl(entry.getKey(), entry.getValue(), false, accessOrder);
         }
     }
 
@@ -127,32 +128,21 @@ public class TLinkedHashMap<K, V> extends THashMap<K, V> implements TSequencedMa
 
     @Override
     public V getOrDefault(Object key, V defaultValue) {
-        LinkedHashMapEntry<K, V> m;
+        LinkedHashMapEntry<K, V> entry;
         if (key == null) {
-            m = (LinkedHashMapEntry<K, V>) findNullKeyEntry();
+            entry = (LinkedHashMapEntry<K, V>) findNullKeyEntry();
         } else {
             int hash = key.hashCode();
             int index = (hash & 0x7FFFFFFF) % elementData.length;
-            m = (LinkedHashMapEntry<K, V>) findNonNullKeyEntry(key, index, hash);
+            entry = (LinkedHashMapEntry<K, V>) findNonNullKeyEntry(key, index, hash);
         }
-        if (m == null) {
+        if (entry == null) {
             return defaultValue;
         }
-        if (accessOrder && tail != m) {
+        if (accessOrder) {
-            LinkedHashMapEntry<K, V> p = m.chainBackward;
+            linkEntry(entry, false);
-            LinkedHashMapEntry<K, V> n = m.chainForward;
-            n.chainBackward = p;
-            if (p != null) {
-                p.chainForward = n;
-            } else {
-                head = n;
-            }
-            m.chainForward = null;
-            m.chainBackward = tail;
-            tail.chainForward = m;
-            tail = m;
         }
-        return m.value;
+        return entry.value;
     }
 
     @Override
@@ -161,128 +151,103 @@ public class TLinkedHashMap<K, V> extends THashMap<K, V> implements TSequencedMa
     }
 
     private HashEntry<K, V> createHashedEntry(K key, int index, int hash, boolean first) {
-        LinkedHashMapEntry<K, V> m = new LinkedHashMapEntry<>(key, hash);
+        var entry = new LinkedHashMapEntry<K, V>(key, hash);
-        m.next = elementData[index];
+        entry.next = elementData[index];
-        elementData[index] = m;
+        elementData[index] = entry;
-        linkEntry(m, first);
+        if (first) {
-        return m;
+            if (head != null) {
+                head.chainBackward = entry;
+            } else {
+                tail = entry;
+            }
+            entry.chainForward = head;
+            head = entry;
+        } else {
+            if (tail != null) {
+                tail.chainForward = entry;
+            } else {
+                head = entry;
+            }
+            entry.chainBackward = tail;
+            tail = entry;
+        }
+        return entry;
     }
 
     @Override
     public V put(K key, V value) {
-        return putImpl(key, value, false);
+        var oldSize = size();
+        var existing = putImpl(key, value, false, accessOrder);
+        if (size() != oldSize) {
+            if (removeEldestEntry(head)) {
+                removeLinkedEntry(head);
+            }
+        }
+        return existing;
     }
 
-    V putImpl(K key, V value, boolean first) {
+    V putImpl(K key, V value, boolean first, boolean forceMotion) {
-        LinkedHashMapEntry<K, V> m;
+        LinkedHashMapEntry<K, V> entry;
         if (elementCount == 0) {
             head = null;
             tail = null;
         }
-        if (key == null) {
+        int hash = Objects.hashCode(key);
-            m = (LinkedHashMapEntry<K, V>) findNullKeyEntry();
+        int index = (hash & Integer.MAX_VALUE) % elementData.length;
-            if (m == null) {
+        entry = (LinkedHashMapEntry<K, V>) (key != null ? findNonNullKeyEntry(key, index, hash) : findNullKeyEntry());
-                modCount++;
+        if (entry == null) {
-                // Check if we need to remove the oldest entry. The check
+            modCount++;
-                // includes accessOrder since an accessOrder LinkedHashMap does
+            if (++elementCount > threshold) {
-                // not record the oldest member in 'head'.
+                rehash();
-                if (++elementCount > threshold) {
+                index = (hash & Integer.MAX_VALUE) % elementData.length;
-                    rehash();
-                }
-                m = (LinkedHashMapEntry<K, V>) createHashedEntry(null, 0, 0, first);
-            } else {
-                linkEntry(m, first);
-            }
-        } else {
-            int hash = key.hashCode();
-            int index = (hash & Integer.MAX_VALUE) % elementData.length;
-            m = (LinkedHashMapEntry<K, V>) findNonNullKeyEntry(key, index, hash);
-            if (m == null) {
-                modCount++;
-                if (++elementCount > threshold) {
-                    rehash();
-                    index = (hash & Integer.MAX_VALUE) % elementData.length;
-                }
-                m = (LinkedHashMapEntry<K, V>) createHashedEntry(key, index, hash, first);
-            } else {
-                linkEntry(m, first);
             }
+            entry = (LinkedHashMapEntry<K, V>) createHashedEntry(key, index, hash, first);
+        } else if (forceMotion) {
+            linkEntry(entry, first);
         }
 
-        V result = m.value;
+        var existing = entry.value;
-        m.value = value;
+        entry.value = value;
-
+        return existing;
-        if (removeEldestEntry(head)) {
-            remove(head.key);
-        }
-
-        return result;
     }
 
-    void linkEntry(LinkedHashMapEntry<K, V> m, boolean first) {
+    private void linkEntry(LinkedHashMapEntry<K, V> entry, boolean first) {
-        if (head == null) {
+        if (first) {
-            // Check if the map is empty
+            var p = entry.chainBackward;
-            head = m;
+            if (p == null) {
-            tail = m;
+                return;
-            return;
+            }
-        }
+            var n = entry.chainForward;
-
-        // we need to link the new entry into either the head or tail
-        // of the chain depending on if the LinkedHashMap is accessOrder or not
-        LinkedHashMapEntry<K, V> p = m.chainBackward;
-        LinkedHashMapEntry<K, V> n = m.chainForward;
-        if (p == null) {
             if (n != null) {
-                // Existing entry must be the head but not the tail
+                n.chainBackward = p;
-                if (!first && accessOrder) {
-                    head = n;
-                    n.chainBackward = null;
-                    m.chainBackward = tail;
-                    m.chainForward = null;
-                    tail.chainForward = m;
-                    tail = m;
-                }
             } else {
-                // This is a new entry
+                tail = p;
-                m.chainBackward = first ? null : tail;
-                m.chainForward = first ? head : null;
-                if (first) {
-                    head.chainBackward = m;
-                    head = m;
-                } else {
-                    tail.chainForward = m;
-                    tail = m;
-                }
             }
+            p.chainForward = n;
+            if (head != null) {
+                head.chainBackward = entry;
+            }
+            entry.chainForward = head;
+            entry.chainBackward = null;
+            head = entry;
         } else {
+            var n = entry.chainForward;
             if (n == null) {
-                // Existing entry must be the tail but not the head
+                return;
-                if (first && accessOrder) {
-                    tail = p;
-                    p.chainForward = null;
-                    m.chainBackward = null;
-                    m.chainForward = head;
-                    head.chainBackward = m;
-                    head = m;
-                }
-            } else {
-                if (elementCount > 1 && accessOrder) {
-                    // Existing entry is neither the head nor tail
-                    p.chainForward = n;
-                    n.chainBackward = p;
-                    if (first) {
-                        m.chainForward = head;
-                        m.chainBackward = null;
-                        head.chainBackward = m;
-                        head = m;
-                    } else {
-                        m.chainForward = null;
-                        m.chainBackward = tail;
-                        tail.chainForward = m;
-                        tail = m;
-                    }
-                }
             }
+            var p = entry.chainBackward;
+            if (p != null) {
+                p.chainForward = n;
+            } else {
+                head = n;
+            }
+            n.chainBackward = p;
+            if (tail != null) {
+                tail.chainForward = entry;
+            }
+            entry.chainBackward = tail;
+            entry.chainForward = null;
+            tail = entry;
         }
     }
 
@@ -324,12 +289,23 @@ public class TLinkedHashMap<K, V> extends THashMap<K, V> implements TSequencedMa
 
     @Override
     public V remove(Object key) {
-        LinkedHashMapEntry<K, V> m = (LinkedHashMapEntry<K, V>) removeByKey(key);
+        var m = (LinkedHashMapEntry<K, V>) removeByKey(key);
         if (m == null) {
             return null;
         }
-        LinkedHashMapEntry<K, V> p = m.chainBackward;
+        unlinkEntry(m);
-        LinkedHashMapEntry<K, V> n = m.chainForward;
+
+        return m.value;
+    }
+
+    void removeLinkedEntry(LinkedHashMapEntry<K, V> entry) {
+        removeEntry(entry);
+        unlinkEntry(entry);
+    }
+
+    private void unlinkEntry(LinkedHashMapEntry<K, V> entry) {
+        var p = entry.chainBackward;
+        var n = entry.chainForward;
         if (p != null) {
             p.chainForward = n;
             if (n != null) {
@@ -345,7 +321,6 @@ public class TLinkedHashMap<K, V> extends THashMap<K, V> implements TSequencedMa
                 tail = null;
             }
         }
-        return m.value;
     }
 
     @Override
@@ -391,12 +366,12 @@ public class TLinkedHashMap<K, V> extends THashMap<K, V> implements TSequencedMa
 
     @Override
     public V putFirst(K k, V v) {
-        return putImpl(k, v, true);
+        return putImpl(k, v, true, true);
     }
 
     @Override
     public V putLast(K k, V v) {
-        return putImpl(k, v, false);
+        return putImpl(k, v, false, true);
     }
 
     @Override

classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMapIterator.java

@@ -53,7 +53,7 @@ class TLinkedHashMapIterator<K, V> {
             throw new IllegalStateException();
         }
         checkConcurrentMod();
-        base.remove(currentEntry.key);
+        base.removeLinkedEntry(currentEntry);
         currentEntry = null;
         expectedModCount++;
     }

tests/src/test/java/org/teavm/classlib/java/util/LinkedHashMapTest.java

@@ -59,8 +59,7 @@ import org.junit.runner.RunWith;
 import org.teavm.classlib.support.MapTest2Support;
 import org.teavm.junit.TeaVMTestRunner;
 
-@SuppressWarnings({ "UnnecessaryUnboxing", "ClassInitializerMayBeStatic",
+@SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
-        "MismatchedQueryAndUpdateOfCollection" })
 @RunWith(TeaVMTestRunner.class)
 public class LinkedHashMapTest {
 
@@ -555,7 +554,7 @@ public class LinkedHashMapTest {
 
     @Test
     public void testSequencedMap() {
-        SequencedMap<Integer, String> map = generateMap();
+        var map = generateMap();
         assertEquals(Map.entry(1, "1"), map.pollFirstEntry());
         assertArrayEquals(new Integer[] { 6, 2, 5, 3, 4 }, map.keySet().toArray(new Integer[0]));
         assertEquals(Map.entry(4, "4"), map.pollLastEntry());
@@ -568,6 +567,7 @@ public class LinkedHashMapTest {
         map.put(7, "7");
         map.putLast(3, "3");
         assertArrayEquals(new Integer[] { 1, 6, 2, 5, 7, 3 }, map.keySet().toArray(new Integer[0]));
+
         map = generateMap().reversed();
         assertEquals(Map.entry(4, "4"), map.pollFirstEntry());
         assertArrayEquals(new Integer[] { 3, 5, 2, 6, 1 }, map.keySet().toArray(new Integer[0]));
@@ -581,12 +581,14 @@ public class LinkedHashMapTest {
         map.put(7, "7");
         map.putLast(6, "6");
         assertArrayEquals(new Integer[] { 7, 1, 3, 5, 2, 6 }, map.keySet().toArray(new Integer[0]));
+
         map = generateAccessOrderMap();
         map.putFirst(3, "3");
         map.put(5, "5");
         map.putLast(2, "2");
         assertArrayEquals(new Integer[] { 3, 1, 6, 4, 5, 2 }, map.keySet().toArray(new Integer[0]));
         assertArrayEquals(new Integer[] { 2, 5, 4, 6, 1, 3 }, map.reversed().keySet().toArray(new Integer[0]));
+
         map = generateAccessOrderMap();
         map.putFirst(1, "1");
         assertArrayEquals(new Integer[] { 1, 6, 2, 5, 3, 4 }, map.keySet().toArray(new Integer[0]));
@@ -597,6 +599,7 @@ public class LinkedHashMapTest {
         map.putFirst(6, "6");
         assertArrayEquals(new Integer[] { 6, 2, 5, 3, 4, 1 }, map.keySet().toArray(new Integer[0]));
         assertArrayEquals(new Integer[] { 1, 4, 3, 5, 2, 6 }, map.reversed().keySet().toArray(new Integer[0]));
+
         map = generateAccessOrderMap().reversed();
         assertArrayEquals(new Integer[] { 4, 3, 5, 2, 6, 1 }, map.keySet().toArray(new Integer[0]));
         map.putFirst(1, "1");
@@ -670,4 +673,38 @@ public class LinkedHashMapTest {
             // ok
         }
     }
+
+    @Test
+    public void reinsertPutDoesNotChangeOrder() {
+        var map = new LinkedHashMap<String, String>();
+        map.put("a", "1");
+        map.put("b", "2");
+        assertArrayEquals(new String[] { "1", "2" }, map.values().toArray(new String[0]));
+
+        map.put("a", "3");
+        assertArrayEquals(new String[] { "3", "2" }, map.values().toArray(new String[0]));
+    }
+
+    @Test
+    public void removesEldestEntry() {
+        var map = new LinkedHashMap<String, String>() {
+            @Override
+            protected boolean removeEldestEntry(Entry<String, String> eldest) {
+                return size() > 3;
+            }
+        };
+        map.put("a", "1");
+        map.put("b", "2");
+        map.put("c", "3");
+        assertArrayEquals(new String[] { "1", "2", "3" }, map.values().toArray(new String[0]));
+
+        map.put("c", "4");
+        assertArrayEquals(new String[] { "1", "2", "4" }, map.values().toArray(new String[0]));
+
+        map.put("d", "5");
+        assertArrayEquals(new String[] { "2", "4", "5" }, map.values().toArray(new String[0]));
+
+        map.put("a", "6");
+        assertArrayEquals(new String[] { "4", "5", "6" }, map.values().toArray(new String[0]));
+    }
 }