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

[David Holmes]

On Thu, 4 May 2023 12:50:34 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> In preparation for Lilliput, I'd like to simplify BinList handling a bit.
>> 
>> BinList are a data structure that stores small blocks that had been prematurely deallocated for re-use. Due to the way BinList is implemented, it cannot store blocks smaller than two words. That causes complexities in metaspace and makes estimating memory usage awkward (important for testing). We can eliminate this complexity by shrinking a Block to just one word. 
>> 
>> ----
>> 
>> Details:
>> 
>> A BinList block before: 
>> ``` Block { Block* next; size_t size; }```
>> A BinList block now: 
>> ``` Block { Block* next; }```
>> 
>> We don't need to store the block size, since all blocks in a single sub-list have the same size. In fact, we used the block size only for verification.
>> 
>> With this patch, gross- and net-allocation sizes in Metaspace are the same for 64-bit (see `get_raw_word_size_for_requested_word_size`), which will make writing tests that try to predict metaspace usage based on allocation pattern a lot less onerous to write. For 32-bit, unfortunately, we still need to align allocation sizes to 2 words since allocation addresses need to be aligned to 64-bit.
>
> 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.

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??

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??

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?

src/hotspot/share/memory/metaspace/metaspaceCommon.hpp line 40:

> 38: // Metaspace allocation alignment:
> 39: 
> 40: // Metaspace allocations have to be aligned such that 64bit values are aligned

nit existing: s/64bit/64-bit/

src/hotspot/share/memory/metaspace/metaspaceCommon.hpp line 45:

> 43: //
> 44: // Klass* structures need to be aligned to KlassAlignmentInBytes, but since that is
> 45: // 64 bit, we don't need special handling for allocating Klass*.

nit 64-bit

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

PR Review: https://git.openjdk.org/jdk/pull/13776#pullrequestreview-1416251041
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1187105834
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1187112134
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1187112479
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1187114289
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1187114592