8281631: HashMap copy constructor and putAll can over-allocate table

Reviewed-by: smarks

Author: XenoAmess

src/java.base/share/classes/java/util/HashMap.java

@@ -495,9 +495,9 @@ final void putMapEntries(Map<? extends K, ? extends V> m, boolean evict) {
         int s = m.size();
         if (s > 0) {
             if (table == null) { // pre-size
-                float ft = ((float)s / loadFactor) + 1.0F;
-                int t = ((ft < (float)MAXIMUM_CAPACITY) ?
-                         (int)ft : MAXIMUM_CAPACITY);
+                double dt = Math.ceil(s / (double)loadFactor);
+                int t = ((dt < (double)MAXIMUM_CAPACITY) ?
+                         (int)dt : MAXIMUM_CAPACITY);
                 if (t > threshold)
                     threshold = tableSizeFor(t);
             } else {
@@ -1527,12 +1527,12 @@ private void readObject(ObjectInputStream s)
         } else if (mappings == 0) {
             // use defaults
         } else if (mappings > 0) {
-            float fc = (float)mappings / lf + 1.0f;
-            int cap = ((fc < DEFAULT_INITIAL_CAPACITY) ?
+            double dc = Math.ceil(mappings / (double)lf);
+            int cap = ((dc < DEFAULT_INITIAL_CAPACITY) ?
                        DEFAULT_INITIAL_CAPACITY :
-                       (fc >= MAXIMUM_CAPACITY) ?
+                       (dc >= MAXIMUM_CAPACITY) ?
                        MAXIMUM_CAPACITY :
-                       tableSizeFor((int)fc));
+                       tableSizeFor((int)dc));
             float ft = (float)cap * lf;
             threshold = ((cap < MAXIMUM_CAPACITY && ft < MAXIMUM_CAPACITY) ?
                          (int)ft : Integer.MAX_VALUE);

src/java.base/share/classes/java/util/WeakHashMap.java

@@ -213,9 +213,7 @@ public WeakHashMap(int initialCapacity, float loadFactor) {
         if (loadFactor <= 0 || Float.isNaN(loadFactor))
             throw new IllegalArgumentException("Illegal Load factor: "+
                                                loadFactor);
-        int capacity = 1;
-        while (capacity < initialCapacity)
-            capacity <<= 1;
+        int capacity = HashMap.tableSizeFor(initialCapacity);
         table = newTable(capacity);
         this.loadFactor = loadFactor;
         threshold = (int)(capacity * loadFactor);
@@ -251,7 +249,7 @@ public WeakHashMap() {
      * @since   1.3
      */
     public WeakHashMap(Map<? extends K, ? extends V> m) {
-        this(Math.max((int) ((float)m.size() / DEFAULT_LOAD_FACTOR + 1.0F),
+        this(Math.max((int) Math.ceil(m.size() / (double)DEFAULT_LOAD_FACTOR),
                 DEFAULT_INITIAL_CAPACITY),
              DEFAULT_LOAD_FACTOR);
         putAll(m);
@@ -468,7 +466,7 @@ public V put(K key, V value) {
         modCount++;
         Entry<K,V> e = tab[i];
         tab[i] = new Entry<>(k, value, queue, h, e);
-        if (++size >= threshold)
+        if (++size > threshold)
             resize(tab.length * 2);
         return null;
     }
@@ -557,7 +555,7 @@ public void putAll(Map<? extends K, ? extends V> m) {
          * to at most one extra resize.
          */
         if (numKeysToBeAdded > threshold) {
-            int targetCapacity = (int)(numKeysToBeAdded / loadFactor + 1);
+            int targetCapacity = (int)Math.ceil(numKeysToBeAdded / (double)loadFactor);
             if (targetCapacity > MAXIMUM_CAPACITY)
                 targetCapacity = MAXIMUM_CAPACITY;
             int newCapacity = table.length;

test/jdk/ProblemList.txt

@@ -542,6 +542,7 @@ java/lang/invoke/LFCaching/LFMultiThreadCachingTest.java        8151492 generic-
 java/lang/invoke/LFCaching/LFGarbageCollectedTest.java          8078602 generic-all
 java/lang/invoke/lambda/LambdaFileEncodingSerialization.java    8249079 linux-x64
 java/lang/invoke/RicochetTest.java                              8251969 generic-all
+java/lang/Enum/ConstantDirectoryOptimalCapacity.java            8282120 generic-all
 
 ############################################################################
 

test/jdk/java/util/HashMap/WhiteBoxResizeTest.java

@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2018, Red Hat, Inc. All rights reserved.
+ * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -21,113 +22,299 @@
  * questions.
  */
 
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
 import java.lang.invoke.VarHandle;
+import java.util.AbstractMap;
+import java.util.AbstractSet;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.ThreadLocalRandom;
+import java.util.Set;
+import java.util.WeakHashMap;
+import java.util.function.Consumer;
 import java.util.function.Supplier;
-import java.util.stream.IntStream;
 
-import static java.util.stream.Collectors.toMap;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNull;
 
 /*
  * @test
- * @bug 8210280
+ * @bug 8210280 8281631
  * @modules java.base/java.util:open
- * @summary White box tests for HashMap internals around table resize
+ * @summary White box tests for HashMap-related internals around table sizing
  * @run testng WhiteBoxResizeTest
- * @key randomness
  */
 public class WhiteBoxResizeTest {
-    final ThreadLocalRandom rnd = ThreadLocalRandom.current();
+
     final MethodHandle TABLE_SIZE_FOR;
-    final VarHandle THRESHOLD;
-    final VarHandle TABLE;
+    final VarHandle HM_TABLE;
+    final VarHandle WHM_TABLE;
 
     public WhiteBoxResizeTest() throws ReflectiveOperationException {
-        Class<?> mClass = HashMap.class;
-        String nodeClassName = mClass.getName() + "$Node";
-        Class<?> nodeArrayClass = Class.forName("[L" + nodeClassName + ";");
-        MethodHandles.Lookup lookup = MethodHandles.privateLookupIn(mClass, MethodHandles.lookup());
-        TABLE = lookup.findVarHandle(mClass, "table", nodeArrayClass);
-        this.TABLE_SIZE_FOR = lookup.findStatic(
-                mClass, "tableSizeFor",
-                MethodType.methodType(int.class, int.class));
-        this.THRESHOLD = lookup.findVarHandle(mClass, "threshold", int.class);
+        MethodHandles.Lookup hmlookup = MethodHandles.privateLookupIn(HashMap.class, MethodHandles.lookup());
+        TABLE_SIZE_FOR = hmlookup.findStatic(
+                HashMap.class, "tableSizeFor", MethodType.methodType(int.class, int.class));
+        HM_TABLE = hmlookup.unreflectVarHandle(HashMap.class.getDeclaredField("table"));
+
+        MethodHandles.Lookup whmlookup = MethodHandles.privateLookupIn(WeakHashMap.class, MethodHandles.lookup());
+        WHM_TABLE = whmlookup.unreflectVarHandle(WeakHashMap.class.getDeclaredField("table"));
     }
 
+    /*
+     * utility methods
+     */
+
     int tableSizeFor(int n) {
         try {
             return (int) TABLE_SIZE_FOR.invoke(n);
-        } catch (Throwable t) { throw new AssertionError(t); }
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
     }
 
-    Object[] table(HashMap map) {
+    Object[] table(Map<?, ?> map) {
         try {
-            return (Object[]) TABLE.get(map);
-        } catch (Throwable t) { throw new AssertionError(t); }
+            VarHandle vh = map instanceof WeakHashMap ? WHM_TABLE : HM_TABLE;
+            return (Object[]) vh.get(map);
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
     }
 
-    int capacity(HashMap map) {
+    int capacity(Map<?, ?> map) {
         return table(map).length;
     }
 
-    @Test
-    public void testTableSizeFor() {
-        assertEquals(tableSizeFor(0), 1);
-        assertEquals(tableSizeFor(1), 1);
-        assertEquals(tableSizeFor(2), 2);
-        assertEquals(tableSizeFor(3), 4);
-        assertEquals(tableSizeFor(15), 16);
-        assertEquals(tableSizeFor(16), 16);
-        assertEquals(tableSizeFor(17), 32);
-        int maxSize = 1 << 30;
-        assertEquals(tableSizeFor(maxSize - 1), maxSize);
-        assertEquals(tableSizeFor(maxSize), maxSize);
-        assertEquals(tableSizeFor(maxSize + 1), maxSize);
-        assertEquals(tableSizeFor(Integer.MAX_VALUE), maxSize);
+    // creates a map with size mappings
+    Map<String, String> makeMap(int size) {
+        Map<String, String> map = new HashMap<>();
+        putN(map, size);
+        return map;
+    }
+
+    // creates a "fake" map: size() returns the given size, but
+    // the entrySet iterator returns only one entry
+    Map<String, String> fakeMap(int size) {
+        return new AbstractMap<>() {
+            public Set<Map.Entry<String, String>> entrySet() {
+                return new AbstractSet<Map.Entry<String, String>>() {
+                    public int size() {
+                        return size;
+                    }
+
+                    public Iterator<Map.Entry<String, String>> iterator() {
+                        return Set.of(Map.entry("1", "1")).iterator();
+                    }
+                };
+            }
+        };
+    }
+
+    void putN(Map<String, String> map, int n) {
+        for (int i = 0; i < n; i++) {
+            String string = Integer.toString(i);
+            map.put(string, string);
+        }
+    }
+
+    /*
+     * tests of tableSizeFor
+     */
+
+    @DataProvider(name = "tableSizeFor")
+    public Object[][] tableSizeForCases() {
+        final int MAX = 1 << 30;
+        return new Object[][] {
+                // tableSizeFor(arg), expected
+                { 0,                   1 },
+                { 1,                   1 },
+                { 2,                   2 },
+                { 3,                   4 },
+                { 4,                   4 },
+                { 5,                   8 },
+                { 15,                 16 },
+                { 16,                 16 },
+                { 17,                 32 },
+                { MAX-1,             MAX },
+                { MAX,               MAX },
+                { MAX+1,             MAX },
+                { Integer.MAX_VALUE, MAX }
+        };
+    }
+
+    @Test(dataProvider = "tableSizeFor")
+    public void tableSizeFor(int arg, int expected) {
+        assertEquals(tableSizeFor(arg), expected);
     }
 
-    @Test
-    public void capacityTestDefaultConstructor() {
-        capacityTestDefaultConstructor(new HashMap<>());
-        capacityTestDefaultConstructor(new LinkedHashMap<>());
+    /*
+     * tests for lazy table allocation
+     */
+
+    @DataProvider(name = "lazy")
+    public Object[][] lazyTableAllocationCases() {
+        return new Object[][]{
+                {new HashMap<>()},
+                // { new WeakHashMap<>() }, // WHM doesn't allocate lazily
+                {new LinkedHashMap<>()}
+        };
     }
 
-    void capacityTestDefaultConstructor(HashMap<Integer, Integer> map) {
+    @Test(dataProvider = "lazy")
+    public void lazyTableAllocation(Map<?, ?> map) {
         assertNull(table(map));
+    }
 
-        map.put(1, 1);
-        assertEquals(capacity(map), 16); // default initial capacity
+    /*
+     * tests for default capacity (no-arg constructor)
+     */
 
-        map.putAll(IntStream.range(0, 64).boxed().collect(toMap(i -> i, i -> i)));
-        assertEquals(capacity(map), 128);
+    @DataProvider(name = "defaultCapacity")
+    public Object[][] defaultCapacityCases() {
+        return new Supplier<?>[][]{
+                {() -> new HashMap<>()},
+                {() -> new LinkedHashMap<>()},
+                {() -> new WeakHashMap<>()}
+        };
     }
 
-    @Test
-    public void capacityTestInitialCapacity() {
-        int initialCapacity = rnd.nextInt(2, 128);
-        List<Supplier<HashMap<Integer, Integer>>> suppliers = List.of(
-            () -> new HashMap<>(initialCapacity),
-            () -> new HashMap<>(initialCapacity, 0.75f),
-            () -> new LinkedHashMap<>(initialCapacity),
-            () -> new LinkedHashMap<>(initialCapacity, 0.75f));
+    @Test(dataProvider = "defaultCapacity")
+    public void defaultCapacity(Supplier<Map<String, String>> s) {
+        Map<String, String> map = s.get();
+        map.put("", "");
+        assertEquals(capacity(map), 16);
+    }
 
-        for (Supplier<HashMap<Integer, Integer>> supplier : suppliers) {
-            HashMap<Integer, Integer> map = supplier.get();
-            assertNull(table(map));
+    /*
+     * tests for requested capacity (int and int+float constructors)
+     */
 
-            map.put(1, 1);
-            assertEquals(capacity(map), tableSizeFor(initialCapacity));
+    @DataProvider(name = "requestedCapacity")
+    public Iterator<Object[]> requestedCapacityCases() {
+        ArrayList<Object[]> cases = new ArrayList<>();
+        for (int i = 2; i < 128; i++) {
+            int cap = i;
+            cases.add(new Object[]{"rhm1", cap, (Supplier<Map<String, String>>) () -> new HashMap<>(cap)});
+            cases.add(new Object[]{"rhm2", cap, (Supplier<Map<String, String>>) () -> new HashMap<>(cap, 0.75f)});
+            cases.add(new Object[]{"rlm1", cap, (Supplier<Map<String, String>>) () -> new LinkedHashMap<>(cap)});
+            cases.add(new Object[]{"rlm2", cap, (Supplier<Map<String, String>>) () -> new LinkedHashMap<>(cap, 0.75f)});
+            cases.add(new Object[]{"rwm1", cap, (Supplier<Map<String, String>>) () -> new WeakHashMap<>(cap)});
+            cases.add(new Object[]{"rwm2", cap, (Supplier<Map<String, String>>) () -> new WeakHashMap<>(cap, 0.75f)});
         }
+        return cases.iterator();
+    }
+
+    @Test(dataProvider = "requestedCapacity")
+    public void requestedCapacity(String label, int cap, Supplier<Map<String, String>> s) {
+        Map<String, String> map = s.get();
+        map.put("", "");
+        assertEquals(capacity(map), tableSizeFor(cap));
+    }
+
+    /*
+     * Tests for capacity after map is populated with a given number N of mappings.
+     * Maps are populated using a copy constructor on a map with N mappings,
+     * other constructors followed by N put() calls, and other constructors followed
+     * by putAll() on a map with N mappings.
+     *
+     * String labels encode the test case for ease of diagnosis if one of the test cases fails.
+     * For example, "plm2pn" is "populated LinkedHashMap, 2-arg constructor, followed by putN".
+     */
+
+    // helper method for one populated capacity case, to provide target types for lambdas
+    Object[] pcc(String label,
+                 int size,
+                 int expectedCapacity,
+                 Supplier<Map<String, String>> supplier,
+                 Consumer<Map<String, String>> consumer) {
+        return new Object[]{label, size, expectedCapacity, supplier, consumer};
+    }
+
+    List<Object[]> genPopulatedCapacityCases(int size, int cap) {
+        return Arrays.asList(
+                pcc("phmcpy", size, cap, () -> new HashMap<>(makeMap(size)),       map -> { }),
+                pcc("phm0pn", size, cap, () -> new HashMap<>(),                    map -> { putN(map, size); }),
+                pcc("phm1pn", size, cap, () -> new HashMap<>(cap),                 map -> { putN(map, size); }),
+                pcc("phm2pn", size, cap, () -> new HashMap<>(cap, 0.75f),          map -> { putN(map, size); }),
+                pcc("phm0pa", size, cap, () -> new HashMap<>(),                    map -> { map.putAll(makeMap(size)); }),
+                pcc("phm1pa", size, cap, () -> new HashMap<>(cap),                 map -> { map.putAll(makeMap(size)); }),
+                pcc("phm2pa", size, cap, () -> new HashMap<>(cap, 0.75f),          map -> { map.putAll(makeMap(size)); }),
+
+                pcc("plmcpy", size, cap, () -> new LinkedHashMap<>(makeMap(size)), map -> { }),
+                pcc("plm0pn", size, cap, () -> new LinkedHashMap<>(),              map -> { putN(map, size); }),
+                pcc("plm1pn", size, cap, () -> new LinkedHashMap<>(cap),           map -> { putN(map, size); }),
+                pcc("plm2pn", size, cap, () -> new LinkedHashMap<>(cap, 0.75f),    map -> { putN(map, size); }),
+                pcc("plm0pa", size, cap, () -> new LinkedHashMap<>(),              map -> { map.putAll(makeMap(size)); }),
+                pcc("plm1pa", size, cap, () -> new LinkedHashMap<>(cap),           map -> { map.putAll(makeMap(size)); }),
+                pcc("plm2pa", size, cap, () -> new LinkedHashMap<>(cap, 0.75f),    map -> { map.putAll(makeMap(size)); }),
+
+                pcc("pwmcpy", size, cap, () -> new WeakHashMap<>(makeMap(size)),   map -> { }),
+                pcc("pwm0pn", size, cap, () -> new WeakHashMap<>(),                map -> { putN(map, size); }),
+                pcc("pwm1pn", size, cap, () -> new WeakHashMap<>(cap),             map -> { putN(map, size); }),
+                pcc("pwm2pn", size, cap, () -> new WeakHashMap<>(cap, 0.75f),      map -> { putN(map, size); }),
+                pcc("pwm0pa", size, cap, () -> new WeakHashMap<>(),                map -> { map.putAll(makeMap(size)); }),
+                pcc("pwm1pa", size, cap, () -> new WeakHashMap<>(cap),             map -> { map.putAll(makeMap(size)); }),
+                pcc("pwm2pa", size, cap, () -> new WeakHashMap<>(cap, 0.75f),      map -> { map.putAll(makeMap(size)); })
+        );
+    }
+
+    List<Object[]> genFakePopulatedCapacityCases(int size, int cap) {
+        return Arrays.asList(
+                pcc("fhmcpy", size, cap, () -> new HashMap<>(fakeMap(size)),       map -> { }),
+                pcc("fhm0pa", size, cap, () -> new HashMap<>(),                    map -> { map.putAll(fakeMap(size)); }),
+                pcc("fhm1pa", size, cap, () -> new HashMap<>(cap),                 map -> { map.putAll(fakeMap(size)); }),
+                pcc("fhm2pa", size, cap, () -> new HashMap<>(cap, 0.75f),          map -> { map.putAll(fakeMap(size)); }),
+
+                pcc("flmcpy", size, cap, () -> new LinkedHashMap<>(fakeMap(size)), map -> { }),
+                pcc("flm0pa", size, cap, () -> new LinkedHashMap<>(),              map -> { map.putAll(fakeMap(size)); }),
+                pcc("flm1pa", size, cap, () -> new LinkedHashMap<>(cap),           map -> { map.putAll(fakeMap(size)); }),
+                pcc("flm2pa", size, cap, () -> new LinkedHashMap<>(cap, 0.75f),    map -> { map.putAll(fakeMap(size)); }),
+
+                pcc("fwmcpy", size, cap, () -> new WeakHashMap<>(fakeMap(size)),   map -> { }),
+                // pcc("fwm0pa", size, cap, () -> new WeakHashMap<>(),                map -> { map.putAll(fakeMap(size)); }), // see note
+                pcc("fwm1pa", size, cap, () -> new WeakHashMap<>(cap),             map -> { map.putAll(fakeMap(size)); }),
+                pcc("fwm2pa", size, cap, () -> new WeakHashMap<>(cap, 0.75f),      map -> { map.putAll(fakeMap(size)); })
+        );
+
+        // Test case "fwm0pa" is commented out because WeakHashMap uses a different allocation
+        // policy from the other map implementations: it deliberately under-allocates in this case.
+    }
+
+    @DataProvider(name = "populatedCapacity")
+    public Iterator<Object[]> populatedCapacityCases() {
+        ArrayList<Object[]> cases = new ArrayList<>();
+        cases.addAll(genPopulatedCapacityCases(11,  16));
+        cases.addAll(genPopulatedCapacityCases(12,  16));
+        cases.addAll(genPopulatedCapacityCases(13,  32));
+        cases.addAll(genPopulatedCapacityCases(64, 128));
+
+        // numbers in this range are truncated by a float computation with 0.75f
+        // but can get an exact result with a double computation with 0.75d
+        cases.addAll(genFakePopulatedCapacityCases(25165824, 33554432));
+        cases.addAll(genFakePopulatedCapacityCases(25165825, 67108864));
+        cases.addAll(genFakePopulatedCapacityCases(25165826, 67108864));
+
+        return cases.iterator();
     }
+
+    @Test(dataProvider = "populatedCapacity")
+    public void populatedCapacity(String label, // unused, included for diagnostics
+                                  int size,     // unused, included for diagnostics
+                                  int expectedCapacity,
+                                  Supplier<Map<String, String>> s,
+                                  Consumer<Map<String, String>> c) {
+        Map<String, String> map = s.get();
+        c.accept(map);
+        assertEquals(capacity(map), expectedCapacity);
+    }
+
 }