RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
XenoAmess
duke at openjdk.java.net
Sat Mar 12 01:37:11 UTC 2022
On Fri, 11 Mar 2022 19:40:39 GMT, Stuart Marks <smarks at openjdk.org> wrote:
>> 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.)
@stuart-marks all the changes to the test you requested at last review comments are done. please have a look, thanks!
-------------
PR: https://git.openjdk.java.net/jdk/pull/7431
More information about the core-libs-dev
mailing list