[master] RFR: JDK-8307002: [Lilliput] [Metaspace] Make use of Klass alignment gaps

[Thomas Stuefe]

On Wed, 26 Apr 2023 15:59:34 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> https://github.com/openjdk/lilliput/pull/13 changed nKlass encoding by increasing the Klass alignment gap to 512. Works fine and gives us a nKlass-value-point-to-class ratio of about 1.5:1, which means we get close to the goal of representing each class with a single value of nKlass without changing Klass to a fixed-sized Layout. It has two disadvantages, though. The most obvious one - the memory waste caused by alignment shadows - is addressed by this issue. 
> 
> (The second disadvantage that I don't address in this issue is what John Rose called "naive overalignment", but it has effects on the patch, see below)
> 
> That waste matters little for normal programs: the average waste is 256 bytes per class, and if Lilliput saves 4 bytes per object, we break even if we create more than 64 objects per class on average. We usually do that.
> 
> But class-generation scenarios are a different matter. There, we often only have a single or very few objects per class, and many many more classes, so the numbers don't add up anymore. So we need a way to eliminate the gaps.
> 
> The simple solution is to save non-class metadata from the same class loader into these gaps. This works fine since much of our non-class metadata is very fine granular. Like sand into cracks, they settle into the gaps and fill them nicely. And it is not even complicated since we already have the machinery (the descendant of Coleen's original dark-matter-prevention-structure that we use to manage prematurely deallocated metaspace). 
> 
> ----
> 
> Numbers:
> 
> Memory usage is almost back to baseline now. Volkers insane lambda generation test from JDK-8302154, loading 1 million lambdas, -Xshare:off, shows a reduction of 1.74GB->1.32GB, very close to the original 1.31GB baseline:
> 
> (interesting because scenarios like these will be the worst case from a metaspace perspective - metaspace usage of normal apps is unexciting with or without Lilliput):
> 
> 
> Lilliput unpatched:
> Virtual space:                                    
>   Non-class space:      832,00 MB reserved,     807,12 MB ( 97%) committed,  13 nodes.
>       Class space:        1,00 GB reserved,     977,31 MB ( 95%) committed,  1 nodes.                                                     
>              Both:        1,81 GB reserved,       1,74 GB ( 96%) committed. 
> 
> Lilliput patched:
> Virtual space:                                    
>   Non-class space:      384,00 MB reserved,     370,75 MB ( 97%) committed,  6 nodes.
>       Class space:        1,00 GB reserved,     977,31 MB ( 95%) committed,  1 nodes.                                                     
>              Both:        1,38 GB reserved,       1,32 GB ( 96%) committed. 
> 
> Stock VM:
> Virtual space:                                    
>   Non-class space:      832,00 MB reserved,     822,44 MB ( 99%) committed,  13 nodes.
>       Class space:        1,00 GB reserved,     521,31 MB ( 51%) committed,  1 nodes.
>              Both:        1,81 GB reserved,       1,31 GB ( 72%) committed. 
> 
> 
> ----
> 
> Patch:
> 
> https://github.com/openjdk/lilliput/pull/13 introduced the notion that every MetaspaceArena has an alignment - instead, as in the stock VM - working with one global alignment. This patch reverts much of this code (makes it easier to merge too). Instead, it treats alignment as an allocation option. An arena can satisfy allocations with different alignment requirements and will do its best to salvage the alignment waste.
> 
> That is the core of the patch. The rest is fluff and tests.
> 
> ---
> 
> Notes:
> 
> Numbers are somewhat fluid: at the moment, we use a Klass alignment of 512 and a nKlass size of 21 bits. We will probably change nKlass width to 24 or 26 bits for Lilliputs first milestone.
> 
> And once this patch is in, the disadvantage of memory waste is eliminated, so we could increase the Klass alignment. Both 512 and 1K are good values: on average a Klass is between 400b...1K. A 1K alignment would spread out the nKlass value points more. Shift the nKlass-value-point-to-class ratio closer to 1:1.
> 
> The still unsolved problem is naive overalignment. We may change the encoding scheme of Klass*->nKlass to take that into account. But not in this patch. But this uncertainty is the reason why I opted for a new method `MetaspaceArena::allocate_for_klass` instead of just giving the existing `Metaspace::allocate` method an alignment parameter.

src/hotspot/share/memory/metaspace/freeBlocks.cpp line 56:

> 54:     if (p == nullptr) {
> 55:       p = _tree.remove_block(requested_word_size, &real_size);
> 56:     }

Reviewer info: this is a functional change, arguably a bug upstream: 
`FreeBlocks` manages small blocks in a linear bin list (`_small_blacks`), larger blocks in a binary tree (`_tree`). Before, when trying to allocate a small block from `FreeBlocks`, we only would look into the bin list. But it makes sense to look into the large block tree too if that fails. Later down, we chop the block in two and save off the remainder anyway.

src/hotspot/share/memory/metaspace/metaspaceAlignment.hpp line 61:

> 59:   word_size = align_up(word_size, metaspace::MetaspaceMinAlignmentWords);
> 60: 
> 61:   return word_size;

Reviewer note: both versions of `get_raw_word_size_for_requested_word_size` are equivalent save for the fact that the old one took alignment as a second input parameter.

test/hotspot/gtest/metaspace/test_metaspacearena_stress.cpp line 34:

> 32: #include "memory/metaspace/metaspaceSettings.hpp"
> 33: #include "memory/metaspace/metaspaceStatistics.hpp"
> 34: #include "runtime/mutexLocker.hpp"

Reviewer note: Since I removed the "alignment" property from MetaspaceArena, both `test_metaspacearena.cpp` and `test_metaspacearena_stress.cpp` had been reverted back to its upstream state; hence the seemingly random changes in those files.

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

PR Review Comment: https://git.openjdk.org/lilliput/pull/86#discussion_r1179172320
PR Review Comment: https://git.openjdk.org/lilliput/pull/86#discussion_r1179177259
PR Review Comment: https://git.openjdk.org/lilliput/pull/86#discussion_r1179343749