On Fri, 11 Mar 2022 15:56:26 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 two additional
> commits since the last revision:
>
> - change KeyStructure to String
> - fix test
Regarding the number of test cases for `tableSizeForCases` and
`populatedCapacityCases`... the issue is not necessarily with execution time
(although if the test is slow it is a problem). The issues are more around: Is
this testing anything useful, and does this make sense to the reader?
I think we can rely on the monotonicity of these functions. If populating a map
both with 49 and with 96 mappings results in a table length of 128, we don't
need to test that all the intermediate inputs also result in a table length of
128. Including all the intermediate inputs makes the source code more bulky and
requires future readers/maintainers to hunt around in the long list of tests to
figure out which ones are significant. Really, the only ones that are
significant are the boundary cases, so just keep those. Adding more tests that
aren't relevant actually does hurt, even if they execute quickly. So: please
cut out all the extra test cases that aren't near the boundary cases.
I'm not sure what's going on with the build. The builds are in GitHub Actions
and they aren't necessarily reliable, so I wouldn't worry about them too much.
I'll run the final version through our internal build/test system before
integration. (I've also done this previously, and the results were fine.)
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 61:
> 59: String String = Integer.toString(i);
> 60: map.put(String, String);
> 61: }
Small details here... the `String` variable should be lower case. But really
this method should be inlined into `putN`, since that's the method that needs
to generate mappings. Then, `makeMap` should call `putN`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 107:
> 105: for (int i = 0; i < size; ++i) {
> 106: putMap(map, i);
> 107: }
Replace this loop with a call to `putN`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 131:
> 129: void putN(Map<String, String> map, int n) {
> 130: for (int i = 0; i < n; i++) {
> 131: putMap(map, i);
Inline `putMap` here.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 317:
> 315: public void defaultCapacity(Supplier<Map<String, String>> s) {
> 316: Map<String, String> map = s.get();
> 317: putMap(map, 0);
`map.put("", "")`
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 343:
> 341: public void requestedCapacity(String label, int cap,
> Supplier<Map<String, String>> s) {
> 342: Map<String, String> map = s.get();
> 343: putMap(map, 0);
No need to generate a key/value pair here, just use string literals, e.g.
`map.put("", "")`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 430:
> 428: map.putAll(makeMap(size));
> 429: })
> 430: );
Wait, did this get reindented? Adding line breaks totally destroys the tabular
nature of test data. Please restore. Yes, the lines end up being longer than
the usual limit, but the benefit of having things in a nice table outweighs to
cost of having the lines be somewhat over-limit.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7431
