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

XenoAmess duke at openjdk.java.net
Thu Feb 17 00:27:06 UTC 2022


On Tue, 15 Feb 2022 03:45:19 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>>> The changes to j.l.Class and the EnumConstantDirectory test don't belong here -- these are _uses_ of HashMap. This bug and fix should focus on HashMap itself, to ensure that the cases in question allocate a table of the right size.
>> 
>> A test will fail if not change codes there. Every pr should pass ci, so I have no choice.
>> 
>>> Are there any other maps that have this computation besides HashMap and WeakHashMap?
>> 
>> Good question. Can I get a list of classes where I should check?(I guesd I shall start at LinkedHashMap and hash sets, but have no further ideas)
>> 
>>> There should be a regression test for this. It's probably sufficient to base this on your original test program, which puts 12 entries into a HashMap using a variety of techniques. It should assert that the table size is 16 in all cases. Also, should there be a test case for WeakHashMap?
>> 
>> OK I will have a try... a hard part is how to read private field in class but I think I can find some clue in the existed tests.
>> 
>>> Also, I changed the summary of the bug report to be more precise. The PR title will need to be changed to correspond to it. Thanks.
>> 
>> OK.
>
>> A test will fail if not change codes there. Every pr should pass ci, so I have no choice.
> 
> Hm, yes I recall in the preliminary email that there was some mention of a test. However, the test seemed to use the same (incorrect) calculation, so maybe the test needs to be fixed instead.
> 
>> Good question. Can I get a list of classes where I should check?(I guesd I shall start at LinkedHashMap and hash sets, but have no further ideas)
> 
> Offhand, the HashMap/LinkedHashMap and the corresponding Set classes, and WeakHashMap, are the main places to look. IdentityHashMap and the Map.of() implementations use a different organization so are probably unrelated. ConcurrentHashMap is another obvious place; you might want to investigate there, but depending on the fix (if any) we might want to handle it separately. I'd search for "loadFactor" or "LOAD_FACTOR" and see if anything else turns up.
> 
>> OK I will have a try... a hard part is how to read private field in class but I think I can find some clue in the existed tests.
> 
> There is an incantation in the test header to ensure that the java.base module is opened for reflection. Let me know if you have trouble with it.

@stuart-marks done. please review again. thanks.

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

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


More information about the core-libs-dev mailing list