[Java] Fix Object2IntHashMap#merge resizing the map while updating …

…an existing key. Fixes #303
real-logic · Oct 1, 2024 · 94068ea · 94068ea
1 parent f6c6d8b
commit 94068ea
Show file tree

Hide file tree

Showing 7 changed files with 404 additions and 200 deletions.
diff --git a/agrona/src/main/java/org/agrona/collections/Int2IntHashMap.java b/agrona/src/main/java/org/agrona/collections/Int2IntHashMap.java
@@ -476,7 +476,7 @@ public int computeIfAbsent(final int key, final IntUnaryOperator mappingFunction
         {
             entries[index] = key;
             entries[index + 1] = value;
-            size++;
+            ++size;
             increaseCapacity();
         }
 
@@ -513,7 +513,7 @@ public int computeIfPresent(final int key, final IntBinaryOperator remappingFunc
         {
             value = remappingFunction.applyAsInt(key, value);
             entries[index + 1] = value;
-            if (value == missingValue)
+            if (missingValue == value)
             {
                 size--;
                 compactChain(index);
@@ -556,7 +556,7 @@ public int compute(final int key, final IntBinaryOperator remappingFunction)
             if (oldValue == missingValue)
             {
                 entries[index] = key;
-                size++;
+                ++size;
                 increaseCapacity();
             }
         }
@@ -830,10 +830,10 @@ public int merge(final int key, final int value, final IntIntFunction remappingF
         if (missingValue != newValue)
         {
             entries[index + 1] = newValue;
-            if (oldValue == missingValue)
+            if (missingValue == oldValue)
             {
                 entries[index] = key;
-                size++;
+                ++size;
                 increaseCapacity();
             }
         }

diff --git a/agrona/src/main/java/org/agrona/collections/Object2IntHashMap.java b/agrona/src/main/java/org/agrona/collections/Object2IntHashMap.java
@@ -323,10 +323,8 @@ public int computeIfAbsent(final K key, final ToIntFunction<? super K> mappingFu
         {
             keys[index] = key;
             values[index] = newValue;
-            if (++size > resizeThreshold)
-            {
-                increaseCapacity();
-            }
+            ++size;
+            increaseCapacity();
         }
 
         return newValue;
@@ -417,10 +415,8 @@ public int compute(final K key, final ObjectIntToIntFunction<? super K> remappin
             if (missingValue == oldValue)
             {
                 keys[index] = key;
-                if (++size > resizeThreshold)
-                {
-                    increaseCapacity();
-                }
+                ++size;
+                increaseCapacity();
             }
         }
         else if (missingValue != oldValue)
@@ -480,8 +476,10 @@ public int merge(final K key, final int value, final IntIntFunction remappingFun
         {
             keys[index] = key;
             values[index] = newValue;
-            if (++size > resizeThreshold)
+            if (missingValue == oldValue)
             {
+                keys[index] = key;
+                ++size;
                 increaseCapacity();
             }
         }
@@ -543,10 +541,7 @@ public int put(final K key, final int value)
 
         values[index] = value;
 
-        if (size > resizeThreshold)
-        {
-            increaseCapacity();
-        }
+        increaseCapacity();
 
         return oldValue;
     }
@@ -597,10 +592,8 @@ public int putIfAbsent(final K key, final int value)
         keys[index] = key;
         values[index] = value;
 
-        if (++size > resizeThreshold)
-        {
-            increaseCapacity();
-        }
+        ++size;
+        increaseCapacity();
 
         return missingValue;
     }
@@ -1032,13 +1025,16 @@ public void forEachInt(final ObjIntConsumer<? super K> action)
 
     private void increaseCapacity()
     {
-        @DoNotSub final int newCapacity = values.length << 1;
-        if (newCapacity < 0)
+        if (size > resizeThreshold)
         {
-            throw new IllegalStateException("max capacity reached at size=" + size);
-        }
+            @DoNotSub final int newCapacity = values.length << 1;
+            if (newCapacity < 0)
+            {
+                throw new IllegalStateException("max capacity reached at size=" + size);
+            }
 
-        rehash(newCapacity);
+            rehash(newCapacity);
+        }
     }
 
     private void rehash(@DoNotSub final int newCapacity)

diff --git a/agrona/src/test/java/org/agrona/collections/Int2IntHashMapTest.java b/agrona/src/test/java/org/agrona/collections/Int2IntHashMapTest.java
@@ -467,6 +467,19 @@ void shouldComputeIfPresentBoxed()
         assertThat(map.get(testKey), is(testValue2));
     }
 
+    @Test
+    void computeIfPresentShouldDeleteEntryIfMissingValue()
+    {
+        final int key = 707070707;
+        final int value = Integer.MIN_VALUE;
+        map.put(key, value);
+
+        assertEquals(MISSING_VALUE, map.computeIfPresent(key, (k, v) -> MISSING_VALUE));
+
+        assertEquals(0, map.size());
+        assertEquals(MISSING_VALUE, map.get(key));
+    }
+
     @Test
     void shouldCompute()
     {
@@ -493,9 +506,36 @@ void shouldComputeBoxed()
 
         assertThat(map.compute(testKey, (k, v) -> testValue), is(testValue));
         assertThat(map.get(testKey), is(testValue));
+        assertEquals(1, map.size());
 
         assertThat(map.compute(testKey, (k, v) -> testValue2), is(testValue2));
         assertThat(map.get(testKey), is(testValue2));
+        assertEquals(1, map.size());
+    }
+
+    @Test
+    void computeShouldDeleteKeyMappingIfMissingValue()
+    {
+        final int key = MISSING_VALUE;
+        final int value = 0;
+        map.put(key, value);
+
+        assertEquals(MISSING_VALUE, map.compute(key, (k, v) -> MISSING_VALUE));
+
+        assertEquals(0, map.size());
+    }
+
+    @Test
+    void computeIsANoOpIfKeyIsUnknownAndMissingValue()
+    {
+        final int key = -303;
+        final int value = 404;
+        map.put(key, value);
+
+        assertEquals(MISSING_VALUE, map.compute(999, (k, v) -> MISSING_VALUE));
+
+        assertEquals(1, map.size());
+        assertEquals(value, map.get(key));
     }
 
     @Test
@@ -888,14 +928,16 @@ void mergeShouldReplaceAnExistingValueInTheMap()
         final int key = 42;
         final int oldValue = 0;
         final int value = 5;
-        final int expectedNewValue = 8;
         final IntIntFunction remappingFunction = (val1, val2) -> val1 + val2 + 3;
         map.put(key, oldValue);
 
-        assertEquals(expectedNewValue, map.merge(key, value, remappingFunction));
+        assertEquals(8, map.merge(key, value, remappingFunction));
+        assertEquals(1, map.size());
+        assertEquals(8, map.get(key));
 
+        assertEquals(-6, map.merge(key, -17, remappingFunction));
         assertEquals(1, map.size());
-        assertEquals(expectedNewValue, map.get(key));
+        assertEquals(-6, map.get(key));
     }
 
     @Test
@@ -912,6 +954,14 @@ void mergeShouldRemoveTheExistingMappingIfRemappingToAMissingValue()
         assertEquals(0, map.size());
     }
 
+    @Test
+    void mergeShouldRejectAMissingValue()
+    {
+        final IllegalArgumentException exception =
+            assertThrowsExactly(IllegalArgumentException.class, () -> map.merge(42, MISSING_VALUE, (v1, v2) -> 1000));
+        assertEquals("cannot accept missingValue", exception.getMessage());
+    }
+
     @Test
     void putIfAbsentReturnsAnExistingValue()
     {
@@ -1190,6 +1240,69 @@ void removeIfIntOnEntrySet()
         assertEquals(2, map.size());
     }
 
+    @Test
+    void shouldRemoveAnExistingKeyMapping()
+    {
+        final int key = 42;
+        final int value = -1_000;
+        map.put(key, value);
+
+        assertEquals(value, map.remove(key));
+
+        assertEquals(0, map.size());
+    }
+
+    @Test
+    void shouldNotRemoveAnNonExistingKey()
+    {
+        final int key = 42;
+        final int value = -1_000;
+        map.put(key, value);
+
+        assertEquals(MISSING_VALUE, map.remove(0));
+
+        assertEquals(1, map.size());
+        assertEquals(value, map.get(key));
+    }
+
+    @Test
+    void shouldRemoveByKeyAndValue()
+    {
+        final int key = 42;
+        final int value = -1_000;
+        map.put(key, value);
+
+        assertTrue(map.remove(key, value));
+
+        assertEquals(0, map.size());
+    }
+
+    @Test
+    void shouldNotRemoveIfKeyDoesNotMatch()
+    {
+        final int key = 42;
+        final int value = -1_000;
+        map.put(key, value);
+
+        assertFalse(map.remove(0, value));
+
+        assertEquals(1, map.size());
+        assertEquals(value, map.get(key));
+    }
+
+    @Test
+    void shouldNotRemoveIfValueDoesNotMatch()
+    {
+        final int key = 42;
+        final int value = -1_000;
+        map.put(key, value);
+
+        assertFalse(map.remove(key, 42));
+
+        assertEquals(1, map.size());
+        assertEquals(value, map.get(key));
+    }
+
     private void assertEntryIs(final Entry<Integer, Integer> entry, final int expectedKey, final int expectedValue)
     {
         assertEquals(expectedKey, entry.getKey().intValue());

diff --git a/agrona/src/test/java/org/agrona/collections/Int2ObjectCacheTest.java b/agrona/src/test/java/org/agrona/collections/Int2ObjectCacheTest.java
@@ -439,6 +439,7 @@ void computeReturnsNullAfterRemovingAnExistingMapping()
     @Test
     void computeReturnsNullForAnUnknownKeyIfTheFunctionReturnsNull()
     {
+        cache.put(11, "11");
         final int key = 42;
         final IntObjectToObjectFunction<String, String> remappingFunction = (k, v) ->
         {
@@ -450,6 +451,8 @@ void computeReturnsNullForAnUnknownKeyIfTheFunctionReturnsNull()
         assertNull(cache.compute(key, remappingFunction));
 
         assertFalse(cache.containsKey(key));
+        assertEquals("11", cache.get(11));
+        assertEquals(1, cache.size());
     }
 
     @Test

diff --git a/agrona/src/test/java/org/agrona/collections/Int2ObjectHashMapNotAvoidingAllocationTest.java b/agrona/src/test/java/org/agrona/collections/Int2ObjectHashMapNotAvoidingAllocationTest.java
@@ -32,28 +32,28 @@ Int2ObjectHashMap<String> newMap(final float loadFactor, final int initialCapaci
     @Test
     void valuesIteratorIsNotCached()
     {
-        assertNotSame(intToObjectMap.values().iterator(), intToObjectMap.values().iterator());
+        assertNotSame(map.values().iterator(), map.values().iterator());
     }
 
     @Test
     void keysIteratorIsNotCached()
     {
-        assertNotSame(intToObjectMap.keySet().iterator(), intToObjectMap.keySet().iterator());
+        assertNotSame(map.keySet().iterator(), map.keySet().iterator());
     }
 
     @Test
     void entryIteratorIsNotCached()
     {
-        assertNotSame(intToObjectMap.entrySet().iterator(), intToObjectMap.entrySet().iterator());
+        assertNotSame(map.entrySet().iterator(), map.entrySet().iterator());
     }
 
     @Test
     void entriesAreAllocatedByEntriesIterator()
     {
-        intToObjectMap.put(1, "1");
-        intToObjectMap.put(2, "2");
+        map.put(1, "1");
+        map.put(2, "2");
 
-        final Iterator<Entry<Integer, String>> entryIterator = intToObjectMap.entrySet().iterator();
+        final Iterator<Entry<Integer, String>> entryIterator = map.entrySet().iterator();
         final Entry<Integer, String> entry1 = entryIterator.next();
         final Entry<Integer, String> entry2 = entryIterator.next();