Adjust the capacity computation in Maps.newHashMapWithExpectedSize.

[This JDK change](openjdk/jdk@3e39304) fixed the computation used when the first write to a `HashMap` is `putAll(otherMap)`. Previously that would occasionally lead to an unnecessarily large internal table. That same computation was copied into `Maps.newHashMapWithExpectedSize`, so it should also be adjusted.

Thanks to @XenoAmess for both the JDK fix and the fix here.

Closes #5965.

RELNOTES=`Maps.newHashMapWithExpectedSize` sometimes allocated maps that were larger than they needed to be.
PiperOrigin-RevId: 436464386

Author: XenoAmess

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,14 +142,19 @@ public void testNewLinkedHashMapWithExpectedSize_wontGrow() throws Exception {
     for (int size = 1; size < 200; size++) {
       assertWontGrow(
           size,
+          new LinkedHashMap<>(),
           Maps.newLinkedHashMapWithExpectedSize(size),
           Maps.newLinkedHashMapWithExpectedSize(size));
     }
   }
 
   @GwtIncompatible // reflection
   private static void assertWontGrow(
-      int size, HashMap<Object, Object> map1, HashMap<Object, Object> map2) throws Exception {
+      int size,
+      HashMap<Object, Object> referenceMap,
+      HashMap<Object, Object> map1,
+      HashMap<Object, Object> 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

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
   }

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,14 +142,19 @@ public void testNewLinkedHashMapWithExpectedSize_wontGrow() throws Exception {
     for (int size = 1; size < 200; size++) {
       assertWontGrow(
           size,
+          new LinkedHashMap<>(),
           Maps.newLinkedHashMapWithExpectedSize(size),
           Maps.newLinkedHashMapWithExpectedSize(size));
     }
   }
 
   @GwtIncompatible // reflection
   private static void assertWontGrow(
-      int size, HashMap<Object, Object> map1, HashMap<Object, Object> map2) throws Exception {
+      int size,
+      HashMap<Object, Object> referenceMap,
+      HashMap<Object, Object> map1,
+      HashMap<Object, Object> 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

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
   }