On Thu, 3 Mar 2022 15:46:37 GMT, XenoAmess <d...@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
