From a1355bb2f7c1c809d320ed551a67d69e4d02358a Mon Sep 17 00:00:00 2001 From: Alexey Andreev Date: Thu, 23 Nov 2023 21:19:28 +0100 Subject: [PATCH] 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. --- .../teavm/classlib/java/util/THashMap.java | 27 ++- .../classlib/java/util/TLinkedHashMap.java | 229 ++++++++---------- .../java/util/TLinkedHashMapIterator.java | 2 +- .../classlib/java/util/LinkedHashMapTest.java | 43 +++- 4 files changed, 166 insertions(+), 135 deletions(-) diff --git a/classlib/src/main/java/org/teavm/classlib/java/util/THashMap.java b/classlib/src/main/java/org/teavm/classlib/java/util/THashMap.java index b5901c785..0435e40ad 100644 --- a/classlib/src/main/java/org/teavm/classlib/java/util/THashMap.java +++ b/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 extends TAbstractMap implements TCloneable, TSerializable { @@ -192,10 +196,10 @@ public class THashMap extends TAbstractMap implements TCloneable, TS @Override public boolean remove(Object object) { if (object instanceof TMap.Entry) { - TMap.Entry oEntry = (TMap.Entry) object; - TMap.Entry entry = associatedMap.entryByKey(oEntry.getKey()); + var oEntry = (TMap.Entry) object; + 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 extends TAbstractMap implements TCloneable, TS return null; } + final void removeEntry(HashEntry 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 removeByKey(Object key) { int index = 0; HashEntry entry; diff --git a/classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMap.java b/classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMap.java index 8fe865d84..8fd097b82 100644 --- a/classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMap.java +++ b/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 extends THashMap implements TSequencedMa rehash(capacity); } for (var it = map.entrySet().iterator(); it.hasNext();) { - TMap.Entry entry = it.next(); - putImpl(entry.getKey(), entry.getValue(), false); + var entry = it.next(); + putImpl(entry.getKey(), entry.getValue(), false, accessOrder); } } @@ -127,32 +128,21 @@ public class TLinkedHashMap extends THashMap implements TSequencedMa @Override public V getOrDefault(Object key, V defaultValue) { - LinkedHashMapEntry m; + LinkedHashMapEntry entry; if (key == null) { - m = (LinkedHashMapEntry) findNullKeyEntry(); + entry = (LinkedHashMapEntry) findNullKeyEntry(); } else { int hash = key.hashCode(); int index = (hash & 0x7FFFFFFF) % elementData.length; - m = (LinkedHashMapEntry) findNonNullKeyEntry(key, index, hash); + entry = (LinkedHashMapEntry) findNonNullKeyEntry(key, index, hash); } - if (m == null) { + if (entry == null) { return defaultValue; } - if (accessOrder && tail != m) { - LinkedHashMapEntry p = m.chainBackward; - LinkedHashMapEntry 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; + if (accessOrder) { + linkEntry(entry, false); } - return m.value; + return entry.value; } @Override @@ -161,128 +151,103 @@ public class TLinkedHashMap extends THashMap implements TSequencedMa } private HashEntry createHashedEntry(K key, int index, int hash, boolean first) { - LinkedHashMapEntry m = new LinkedHashMapEntry<>(key, hash); - m.next = elementData[index]; - elementData[index] = m; - linkEntry(m, first); - return m; + var entry = new LinkedHashMapEntry(key, hash); + entry.next = elementData[index]; + elementData[index] = entry; + if (first) { + 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) { - LinkedHashMapEntry m; + V putImpl(K key, V value, boolean first, boolean forceMotion) { + LinkedHashMapEntry entry; if (elementCount == 0) { head = null; tail = null; } - if (key == null) { - m = (LinkedHashMapEntry) findNullKeyEntry(); - if (m == null) { - modCount++; - // Check if we need to remove the oldest entry. The check - // includes accessOrder since an accessOrder LinkedHashMap does - // not record the oldest member in 'head'. - if (++elementCount > threshold) { - rehash(); - } - m = (LinkedHashMapEntry) createHashedEntry(null, 0, 0, first); - } else { - linkEntry(m, first); - } - } else { - int hash = key.hashCode(); - int index = (hash & Integer.MAX_VALUE) % elementData.length; - m = (LinkedHashMapEntry) findNonNullKeyEntry(key, index, hash); - if (m == null) { - modCount++; - if (++elementCount > threshold) { - rehash(); - index = (hash & Integer.MAX_VALUE) % elementData.length; - } - m = (LinkedHashMapEntry) createHashedEntry(key, index, hash, first); - } else { - linkEntry(m, first); + int hash = Objects.hashCode(key); + int index = (hash & Integer.MAX_VALUE) % elementData.length; + entry = (LinkedHashMapEntry) (key != null ? findNonNullKeyEntry(key, index, hash) : findNullKeyEntry()); + if (entry == null) { + modCount++; + if (++elementCount > threshold) { + rehash(); + index = (hash & Integer.MAX_VALUE) % elementData.length; } + entry = (LinkedHashMapEntry) createHashedEntry(key, index, hash, first); + } else if (forceMotion) { + linkEntry(entry, first); } - V result = m.value; - m.value = value; - - if (removeEldestEntry(head)) { - remove(head.key); - } - - return result; + var existing = entry.value; + entry.value = value; + return existing; } - void linkEntry(LinkedHashMapEntry m, boolean first) { - if (head == null) { - // Check if the map is empty - head = m; - tail = m; - return; - } - - // 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 p = m.chainBackward; - LinkedHashMapEntry n = m.chainForward; - if (p == null) { + private void linkEntry(LinkedHashMapEntry entry, boolean first) { + if (first) { + var p = entry.chainBackward; + if (p == null) { + return; + } + var n = entry.chainForward; if (n != null) { - // Existing entry must be the head but not the tail - if (!first && accessOrder) { - head = n; - n.chainBackward = null; - m.chainBackward = tail; - m.chainForward = null; - tail.chainForward = m; - tail = m; - } + n.chainBackward = p; } else { - // This is a new entry - m.chainBackward = first ? null : tail; - m.chainForward = first ? head : null; - if (first) { - head.chainBackward = m; - head = m; - } else { - tail.chainForward = m; - tail = m; - } + tail = p; } + 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 - 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; - } - } + return; } + 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 extends THashMap implements TSequencedMa @Override public V remove(Object key) { - LinkedHashMapEntry m = (LinkedHashMapEntry) removeByKey(key); + var m = (LinkedHashMapEntry) removeByKey(key); if (m == null) { return null; } - LinkedHashMapEntry p = m.chainBackward; - LinkedHashMapEntry n = m.chainForward; + unlinkEntry(m); + + return m.value; + } + + void removeLinkedEntry(LinkedHashMapEntry entry) { + removeEntry(entry); + unlinkEntry(entry); + } + + private void unlinkEntry(LinkedHashMapEntry 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 extends THashMap implements TSequencedMa tail = null; } } - return m.value; } @Override @@ -391,12 +366,12 @@ public class TLinkedHashMap extends THashMap 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 diff --git a/classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMapIterator.java b/classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMapIterator.java index 62de6626f..0a73a0470 100644 --- a/classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMapIterator.java +++ b/classlib/src/main/java/org/teavm/classlib/java/util/TLinkedHashMapIterator.java @@ -53,7 +53,7 @@ class TLinkedHashMapIterator { throw new IllegalStateException(); } checkConcurrentMod(); - base.remove(currentEntry.key); + base.removeLinkedEntry(currentEntry); currentEntry = null; expectedModCount++; } diff --git a/tests/src/test/java/org/teavm/classlib/java/util/LinkedHashMapTest.java b/tests/src/test/java/org/teavm/classlib/java/util/LinkedHashMapTest.java index 76b68ca22..8fbd706e3 100644 --- a/tests/src/test/java/org/teavm/classlib/java/util/LinkedHashMapTest.java +++ b/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", - "MismatchedQueryAndUpdateOfCollection" }) +@SuppressWarnings("MismatchedQueryAndUpdateOfCollection") @RunWith(TeaVMTestRunner.class) public class LinkedHashMapTest { @@ -555,7 +554,7 @@ public class LinkedHashMapTest { @Test public void testSequencedMap() { - SequencedMap 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(); + 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() { + @Override + protected boolean removeEldestEntry(Entry 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])); + } }