diff --git a/android/guava-tests/test/com/google/common/collect/MapsTest.java b/android/guava-tests/test/com/google/common/collect/MapsTest.java index d0f98386346d..96f59d188fbf 100644 --- a/android/guava-tests/test/com/google/common/collect/MapsTest.java +++ b/android/guava-tests/test/com/google/common/collect/MapsTest.java @@ -126,7 +126,10 @@ public void testNewHashMapWithExpectedSize_wontGrow() throws Exception { for (int size = 1; size < 200; size++) { assertWontGrow( - size, Maps.newHashMapWithExpectedSize(size), Maps.newHashMapWithExpectedSize(size)); + size, + new HashMap<>(), + Maps.newHashMapWithExpectedSize(size), + Maps.newHashMapWithExpectedSize(size)); } } @@ -139,6 +142,7 @@ public void testNewLinkedHashMapWithExpectedSize_wontGrow() throws Exception { for (int size = 1; size < 200; size++) { assertWontGrow( size, + new LinkedHashMap<>(), Maps.newLinkedHashMapWithExpectedSize(size), Maps.newLinkedHashMapWithExpectedSize(size)); } @@ -146,7 +150,11 @@ public void testNewLinkedHashMapWithExpectedSize_wontGrow() throws Exception { @GwtIncompatible // reflection private static void assertWontGrow( - int size, HashMap map1, HashMap map2) throws Exception { + int size, + HashMap referenceMap, + HashMap map1, + HashMap map2) + throws Exception { // Only start measuring table size after the first element inserted, to // deal with empty-map optimization. map1.put(0, null); @@ -168,6 +176,16 @@ private static void assertWontGrow( assertWithMessage("table size after adding " + size + " elements") .that(bucketsOf(map1)) .isEqualTo(initialBuckets); + + // Ensure that referenceMap, which doesn't use WithExpectedSize, ends up with the same table + // size as the other two maps. If it ended up with a smaller size that would imply that we + // computed the wrong initial capacity. + for (int i = 0; i < size; i++) { + referenceMap.put(i, null); + } + assertWithMessage("table size after adding " + size + " elements") + .that(initialBuckets) + .isAtMost(bucketsOf(referenceMap)); } @GwtIncompatible // reflection diff --git a/android/guava/src/com/google/common/collect/Maps.java b/android/guava/src/com/google/common/collect/Maps.java index 76e69abb79c0..6235f195eda5 100644 --- a/android/guava/src/com/google/common/collect/Maps.java +++ b/android/guava/src/com/google/common/collect/Maps.java @@ -237,10 +237,19 @@ static int capacity(int expectedSize) { return expectedSize + 1; } if (expectedSize < Ints.MAX_POWER_OF_TWO) { - // This is the calculation used in JDK8 to resize when a putAll - // happens; it seems to be the most conservative calculation we - // can make. 0.75 is the default load factor. - return (int) ((float) expectedSize / 0.75F + 1.0F); + // This seems to be consistent across JDKs. The capacity argument to HashMap and LinkedHashMap + // ends up being used to compute a "threshold" size, beyond which the internal table + // will be resized. That threshold is ceilingPowerOfTwo(capacity*loadFactor), where + // loadFactor is 0.75 by default. So with the calculation here we ensure that the + // threshold is equal to ceilingPowerOfTwo(expectedSize). There is a separate code + // path when the first operation on the new map is putAll(otherMap). There, prior to + // https://github.com/openjdk/jdk/commit/3e393047e12147a81e2899784b943923fc34da8e, a bug + // meant that sometimes a too-large threshold is calculated. However, this new threshold is + // independent of the initial capacity, except that it won't be lower than the threshold + // computed from that capacity. Because the internal table is only allocated on the first + // write, we won't see copying because of the new threshold. So it is always OK to use the + // calculation here. + return (int) Math.ceil(expectedSize / 0.75); } return Integer.MAX_VALUE; // any large value } diff --git a/guava-tests/test/com/google/common/collect/MapsTest.java b/guava-tests/test/com/google/common/collect/MapsTest.java index 50df8cc5f0b7..c8827461b176 100644 --- a/guava-tests/test/com/google/common/collect/MapsTest.java +++ b/guava-tests/test/com/google/common/collect/MapsTest.java @@ -126,7 +126,10 @@ public void testNewHashMapWithExpectedSize_wontGrow() throws Exception { for (int size = 1; size < 200; size++) { assertWontGrow( - size, Maps.newHashMapWithExpectedSize(size), Maps.newHashMapWithExpectedSize(size)); + size, + new HashMap<>(), + Maps.newHashMapWithExpectedSize(size), + Maps.newHashMapWithExpectedSize(size)); } } @@ -139,6 +142,7 @@ public void testNewLinkedHashMapWithExpectedSize_wontGrow() throws Exception { for (int size = 1; size < 200; size++) { assertWontGrow( size, + new LinkedHashMap<>(), Maps.newLinkedHashMapWithExpectedSize(size), Maps.newLinkedHashMapWithExpectedSize(size)); } @@ -146,7 +150,11 @@ public void testNewLinkedHashMapWithExpectedSize_wontGrow() throws Exception { @GwtIncompatible // reflection private static void assertWontGrow( - int size, HashMap map1, HashMap map2) throws Exception { + int size, + HashMap referenceMap, + HashMap map1, + HashMap map2) + throws Exception { // Only start measuring table size after the first element inserted, to // deal with empty-map optimization. map1.put(0, null); @@ -168,6 +176,16 @@ private static void assertWontGrow( assertWithMessage("table size after adding " + size + " elements") .that(bucketsOf(map1)) .isEqualTo(initialBuckets); + + // Ensure that referenceMap, which doesn't use WithExpectedSize, ends up with the same table + // size as the other two maps. If it ended up with a smaller size that would imply that we + // computed the wrong initial capacity. + for (int i = 0; i < size; i++) { + referenceMap.put(i, null); + } + assertWithMessage("table size after adding " + size + " elements") + .that(initialBuckets) + .isAtMost(bucketsOf(referenceMap)); } @GwtIncompatible // reflection diff --git a/guava/src/com/google/common/collect/Maps.java b/guava/src/com/google/common/collect/Maps.java index 6265f5dcee2f..0754dbe332c5 100644 --- a/guava/src/com/google/common/collect/Maps.java +++ b/guava/src/com/google/common/collect/Maps.java @@ -284,10 +284,19 @@ static int capacity(int expectedSize) { return expectedSize + 1; } if (expectedSize < Ints.MAX_POWER_OF_TWO) { - // This is the calculation used in JDK8 to resize when a putAll - // happens; it seems to be the most conservative calculation we - // can make. 0.75 is the default load factor. - return (int) ((float) expectedSize / 0.75F + 1.0F); + // This seems to be consistent across JDKs. The capacity argument to HashMap and LinkedHashMap + // ends up being used to compute a "threshold" size, beyond which the internal table + // will be resized. That threshold is ceilingPowerOfTwo(capacity*loadFactor), where + // loadFactor is 0.75 by default. So with the calculation here we ensure that the + // threshold is equal to ceilingPowerOfTwo(expectedSize). There is a separate code + // path when the first operation on the new map is putAll(otherMap). There, prior to + // https://github.com/openjdk/jdk/commit/3e393047e12147a81e2899784b943923fc34da8e, a bug + // meant that sometimes a too-large threshold is calculated. However, this new threshold is + // independent of the initial capacity, except that it won't be lower than the threshold + // computed from that capacity. Because the internal table is only allocated on the first + // write, we won't see copying because of the new threshold. So it is always OK to use the + // calculation here. + return (int) Math.ceil(expectedSize / 0.75); } return Integer.MAX_VALUE; // any large value }