RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v24]

[Stuart Marks]

On Thu, 3 Mar 2022 15:46:37 GMT, XenoAmess <duke at openjdk.java.net> wrote:

>> 8281631: HashMap copy constructor and putAll can over-allocate table
>
> XenoAmess has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cast several float to double before calculation

OK, I took a look at HashMapsPutAllOverAllocateTableTest.java. It's certainly a good start at testing stuff in this area. However, I notice that 

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

already exists and tests kind-of similar things. I think it would be preferable to enhance this test with additional assertions since it already has a bunch of machinery to inspect the various internals. It tests both HashMap and LinkedHashMap, so I think it would be ok to add a WeakHashMap test here as well. I note however that it relies on LinkedHashMap being a subclass of HashMap, which WeakHashMap is not, and so there will be a certain amount of messiness adding in the handling for WeakHashMap.

Once this is in place though I think adding the additional test cases would fit in well here. In particular, it should be possible to test the proper table lengths for the copy-constructor and putAll cases (from your original bug report) as well as the premature table expansion in WeakHashMap.

This test could use some cleanup as well. For example, the capacityTestInitialCapacity() method has a list of suppliers that it loops over. This is probably better expressed as a data provider driving a test method that tests one case. There's a bit of an art to setting this up. TestNG wants each test case to be `Object[][]` which is kind of a pain if you want to use a lambda. A trick is to have a method that returns `Object[]` representing one test case, and have the parameters of this method provide the target types for the lambdas. For example:

    Object[] testcase(String label, Supplier<Map<Integer,Integer>> s, Consumer<Map<Integer,Integer>> c, int expectedCapacity) {
        return new Object[] { label, s, c, expectedCapacity };
    }

and then have the actual data provider just be a series of calls to this method in an array literal:

    @DataProvider
    public Object[][] allCases() {
        return new Object[][] {
            testcase("HMputAll", () -> new HashMap<>(), m -> m.putAll(MAP12), 16),
            testcase("WHMputAll", () -> new WeakHashMap<>(), m -> m.putAll(MAP12), 16),
            testcase("HMcopyctor", () -> new HashMap<>(MAP12), m -> { }, 16),
            testcase("WHMcopyctor", () -> new WeakHashMap<>(MAP12), m -> { }, 16),
        };
    }

This lets you write the lambdas without a cast that provides the target type.

The test method calls the supplier, passes the map to the consumer, gets the table length, and asserts that it's equal to the expected length. Note that I've set this up with separate supplier and consumer in order to accommodate both the copy constructor cases and the putAll cases. Finally, the label string isn't used by the test method, but it's useful if one of the cases fails. TestNG will print out the arguments of a failing case and the label helps a lot here, as the string form of the lambda is basically unintelligible.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7431