RFR: JDK-8307356: Metaspace: simplify BinList handling [v2]

[Thomas Stuefe]

On Mon, 8 May 2023 07:27:35 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   feedback roman
>
> A few drive-by comments as this isn't an area I'm familiar with.
> 
> I'm unclear if these kind of helper changes should be held off until we know what release Lilliput will be targeted to? Is this change valuable enough without Lilliput?
> 
> Thanks.

Hi @dholmes-ora,

> I'm unclear if these kind of helper changes should be held off until we know what release Lilliput will be targeted to? Is this change valuable enough without Lilliput?

I think so, yes. It will allow me to simplify tests even independently from Lilliput.

> src/hotspot/share/memory/metaspace/binList.hpp line 114:
> 
>> 112:   static size_t word_size_for_index(int index) {
>> 113:     assert(index >= 0 && index < num_lists, "Invalid index %d", index);
>> 114:     return index + MinWordSize;
> 
> Pre-existing but I don't understand what `word_size_for_index` actually means??

Its the reverse of index_for_word_size. Given a slot index that indicates a sub list, tells you what word size is kept in that sub list. Since I obviously don't store blocks of word size 0, the sub list with index 0 keeps block of (now, with this patch) size 1, before it was blocks of size 2.

> src/hotspot/share/memory/metaspace/binList.hpp line 161:
> 
>> 159:       *p_real_word_size = real_word_size;
>> 160:       MetaWord* p = (MetaWord*)b;
>> 161:       assert(real_word_size == 1 || p[real_word_size - 1] == 0,
> 
> Isn't the first case also handled by the second??

This probably would deserve a comment, but I recently got obsessed with patch sizes :/

This line checks the trailing canary of this block that is written in BinList::add_block(). But if the blocks are of size 1, there is no space for the canary (in BinList::add_block(), the canary is overwritten by the 1-word-sized Block Header). Therefore, if the block is size 1, we cannot check the canary.

> src/hotspot/share/memory/metaspace/binList.hpp line 185:
> 
>> 183:       int pos = 0;
>> 184:       for (Block* b = _blocks[i]; b != nullptr; b = b->_next, pos++) {
>> 185:         assert(s == 1 || ((MetaWord*)b)[s - 1] == 0,
> 
> Again isn't the first case handled by the second?

See answer above. I realize I probably should factor this out into a reasonably named method.

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

PR Comment: https://git.openjdk.org/jdk/pull/13776#issuecomment-1537907482
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1187137243
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1187139242
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1187139843