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

[Coleen Phillimore]

On Mon, 8 May 2023 10:04:29 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 David

This looks fine.  I had a couple of nits and questions.

src/hotspot/share/memory/metaspace/binList.hpp line 154:

> 152:     const int index = index_for_word_size(word_size);
> 153:     Block* old_head = _blocks[index];
> 154:     Block* new_head = new(p)Block(old_head);

nit: add spaces around (p)

src/hotspot/share/memory/metaspace/metaspaceArena.cpp line 249:

> 247: 
> 248:   // Primary allocation
> 249:   p = allocate_inner(raw_word_size);

Why wasn't this always raw_word_size which seems to be the aligned size based on the requested word size? ('aligned' might be a better word than 'raw' here)  If I'm reading it correctly.

src/hotspot/share/memory/metaspace/metaspaceArena.cpp line 382:

> 380:   assert_is_aligned_metaspace_pointer(p);
> 381:   size_t raw_word_size = get_raw_word_size_for_requested_word_size(word_size);
> 382: 

If this is deallocating a pointer returned from metaspace, why isn't raw_word_size the same as word_size?

test/hotspot/gtest/metaspace/test_binlist.cpp line 49:

> 47: struct BinListBasicTest {
> 48: 
> 49:     static const size_t maxws;

indentation seems off?

test/hotspot/gtest/metaspace/test_freeblocks.cpp line 102:

> 100:   bool allocate() {
> 101: 
> 102:     size_t word_size = MAX2(_rgen_allocations.get(), size_t(1));

I thought you kept MinWordSize above?

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13776#pullrequestreview-1439980979
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1202541456
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1202561406
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1202563666
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1202571212
PR Review Comment: https://git.openjdk.org/jdk/pull/13776#discussion_r1202575162