From 3e393047e12147a81e2899784b943923fc34da8e Mon Sep 17 00:00:00 2001 From: XenoAmess Date: Wed, 16 Mar 2022 16:48:49 +0000 Subject: [PATCH] 8281631: HashMap copy constructor and putAll can over-allocate table Reviewed-by: smarks --- .../share/classes/java/util/HashMap.java | 14 +- .../share/classes/java/util/WeakHashMap.java | 10 +- test/jdk/ProblemList.txt | 1 + .../java/util/HashMap/WhiteBoxResizeTest.java | 305 ++++++++++++++---- 4 files changed, 258 insertions(+), 72 deletions(-) diff --git a/src/java.base/share/classes/java/util/HashMap.java b/src/java.base/share/classes/java/util/HashMap.java index 24fcd9516ca47..997273aa104f1 100644 --- a/src/java.base/share/classes/java/util/HashMap.java +++ b/src/java.base/share/classes/java/util/HashMap.java @@ -495,9 +495,9 @@ final void putMapEntries(Map 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); diff --git a/src/java.base/share/classes/java/util/WeakHashMap.java b/src/java.base/share/classes/java/util/WeakHashMap.java index d74c7d6572846..bc31985b66ee2 100644 --- a/src/java.base/share/classes/java/util/WeakHashMap.java +++ b/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 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 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 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; diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt index fc1979b6ed635..0bafc13a25291 100644 --- a/test/jdk/ProblemList.txt +++ b/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 ############################################################################ diff --git a/test/jdk/java/util/HashMap/WhiteBoxResizeTest.java b/test/jdk/java/util/HashMap/WhiteBoxResizeTest.java index e7374dff5cd2d..ad02b3000e218 100644 --- a/test/jdk/java/util/HashMap/WhiteBoxResizeTest.java +++ b/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 makeMap(int size) { + Map 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 fakeMap(int size) { + return new AbstractMap<>() { + public Set> entrySet() { + return new AbstractSet>() { + public int size() { + return size; + } + + public Iterator> iterator() { + return Set.of(Map.entry("1", "1")).iterator(); + } + }; + } + }; + } + + void putN(Map 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 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>> 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> s) { + Map map = s.get(); + map.put("", ""); + assertEquals(capacity(map), 16); + } - for (Supplier> supplier : suppliers) { - HashMap 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 requestedCapacityCases() { + ArrayList cases = new ArrayList<>(); + for (int i = 2; i < 128; i++) { + int cap = i; + cases.add(new Object[]{"rhm1", cap, (Supplier>) () -> new HashMap<>(cap)}); + cases.add(new Object[]{"rhm2", cap, (Supplier>) () -> new HashMap<>(cap, 0.75f)}); + cases.add(new Object[]{"rlm1", cap, (Supplier>) () -> new LinkedHashMap<>(cap)}); + cases.add(new Object[]{"rlm2", cap, (Supplier>) () -> new LinkedHashMap<>(cap, 0.75f)}); + cases.add(new Object[]{"rwm1", cap, (Supplier>) () -> new WeakHashMap<>(cap)}); + cases.add(new Object[]{"rwm2", cap, (Supplier>) () -> new WeakHashMap<>(cap, 0.75f)}); } + return cases.iterator(); + } + + @Test(dataProvider = "requestedCapacity") + public void requestedCapacity(String label, int cap, Supplier> s) { + Map 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> supplier, + Consumer> consumer) { + return new Object[]{label, size, expectedCapacity, supplier, consumer}; + } + + List 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 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 populatedCapacityCases() { + ArrayList 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> s, + Consumer> c) { + Map map = s.get(); + c.accept(map); + assertEquals(capacity(map), expectedCapacity); + } + }