[master] RFR: JDK-8325104: Lilliput: Shrink Classpointers [v3]

[Stefan Karlsson]

On Mon, 25 Mar 2024 14:51:14 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi,
>> 
>> I wanted to get input on the following improvement for Lilliput. Testing is still ongoing, but things look really good, so this patch is hopefully near its final form (barring any objections from reviewers, of course).
>> 
>> Note: I have a companion patch prepared for upstream, minus the markword changes. I will attempt to get that one upstream quickly in order to not have a large delta between upstream and lilliput, especially in Metaspace.
>> 
>> ## High-Level Overview
>> 
>> (for a short sequence of slides, please see https://github.com/tstuefe/fosdem24/blob/master/classpointers-and-liliput.pdf - these accompanied a talk we held at FOSDEM 24).
>> 
>> We want to reduce the bit size of narrow Klass to free up bits in the MarkWord. 
>> 
>> We cannot just reduce the Klass encoding range size (well, we could, and maybe we will later, but for now we decided not to). We instead increase the alignment Klass is stored at, and use that alignment shadow to store other information.
>> 
>> In other words, this patch changes the narrow Klass Pointer to a Klass ID, since now (almost) every value in its value range points to a different class. Therefore, we use the value range of nKlass much more efficiently.
>> 
>> We then use the newly freed bits in the MarkWord to restore the iHash to 31 bits: 
>> 
>> 
>> [ 22-bit nKlass | 31-bit iHash | 4 free bits | age | fwd | lck ]
>> 
>> nKlass gets reduced to 22 bits. Identity hash gets re-inflated to 31 bits. Preceding iHash are now 4 unused bits. Rest is unchanged.
>> 
>> (Note: I originally wanted to swap iHash and nKlass such that either of them could be loaded with a 32-bit load, but I found that tricky since C2 seems to rely on the nKlass offset in the Markword being > 0.)
>> 
>> ## nKlass reduction:
>> 
>> The reduction in nKlass size is made by only storing them at 10-bit aligned addresses. That alignment (1KB) works well in practice since Klass - although var sized - typically is between 512 bytes and 1KB in size. Outliers are possible, but the size distribution is bell-curvish [1], so far-away outliers are very rare. 
>> 
>> To not lose memory to alignment waste, metaspace is reshaped to handle arbitrarily aligned allocations efficiently. Basically, we allow the non-Klass arena of a class loader to steal the alignment waste storage from the class arena. So, alignment waste blocks are filled with non-Klass metadata. That works very well in practice since non-Klass metadata is numerous and fine-granular compared to the big Klass bloc...
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits:
> 
>  - Roman feedback - small stuff
>  - Merge branch 'master' into Smaller-ClassPointers
>  - revert COH archive generation
>  - Remove files that accidentally slipped in
>  - Merge branch 'master' into Smaller-ClassPointers
>  - Merge
>  - Merge commit 'c1281e6b45ed167df69d29a6039d81854c145ae6~1' into Smaller-ClassPointers
>  - Fix Typo
>  - Better CDS arch generation
>  - Fix error in COH archive generation
>  - ... and 10 more: https://git.openjdk.org/lilliput/compare/b2fcfb73...1260f2d6

I took another pass over the code and have some more questions.

src/hotspot/share/memory/classLoaderMetaspace.cpp line 103:

> 101: // Allocate word_size words from Metaspace.
> 102: MetaWord* ClassLoaderMetaspace::allocate(size_t word_size, Metaspace::MetadataType mdType) {
> 103:   word_size = align_up(word_size, Metaspace::min_allocation_word_size);

Could you explain why you need to align word_size here? It seems like we perform a very similar alignment inside MetaspaceArena::allocate.

src/hotspot/share/memory/classLoaderMetaspace.cpp line 161:

> 159: // because it is not needed anymore.
> 160: void ClassLoaderMetaspace::deallocate(MetaWord* ptr, size_t word_size, bool is_class) {
> 161:   NOT_LP64(word_size = align_down(word_size, Metaspace::min_allocation_word_size);)

Why is this needed? What code paths passes in non-aligned word_size?

(BTW, I started to look at these in more details because I tend to think that we should stay away from modifying the input parameters, since it makes it easier to misunderstand the code).

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

> 200:         b_last = b;
> 201:       }
> 202:       if (UseNewCode)printf("\n");

This looks like old debugging code.

src/hotspot/share/memory/metaspace/blockTree.cpp line 187:

> 185: void BlockTree::zap_block(MetaBlock bl) {
> 186:   memset(bl.base(), 0xF3, bl.word_size() * sizeof(MetaWord));
> 187: }

In the header file the parameter is named `block` and here it is named `bl`. In `add_block(MetaBlock mb)` the name is `mb`. Maybe think about trying to use consistent naming for these?

src/hotspot/share/memory/metaspace/metablock.hpp line 52:

> 50:   bool is_empty() const { return _base == nullptr; }
> 51:   bool is_nonempty() const { return _base != nullptr; }
> 52:   void reset() { _base = nullptr; _word_size = 0; }

Do you need reset? I was wondering if you could make the _base, _word_size const by changing the few places that use this to: `result = MetaBlock();`

You would also need to skip having `split_off_tail` and replace its usage with:

wastage = MetaBlock(...);
result  = Metablock(...)


Just a thought if you wanted to make MetaBlock an even simpler tiny structure.

src/hotspot/share/memory/metaspace/metablock.inline.hpp line 29:

> 27: #define SHARE_MEMORY_METASPACE_METABLOCK_INLINE_HPP
> 28: 
> 29: #include "memory/metaspace/metablock.hpp"

We have a convention that the .hpp file associated with the .inline.hpp file should come first (as you have here) and then there's a blank line separating it from the rest of the includes.

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

PR Review: https://git.openjdk.org/lilliput/pull/128#pullrequestreview-1962779347
PR Review Comment: https://git.openjdk.org/lilliput/pull/128#discussion_r1540783269
PR Review Comment: https://git.openjdk.org/lilliput/pull/128#discussion_r1540791019
PR Review Comment: https://git.openjdk.org/lilliput/pull/128#discussion_r1540798030
PR Review Comment: https://git.openjdk.org/lilliput/pull/128#discussion_r1540802163
PR Review Comment: https://git.openjdk.org/lilliput/pull/128#discussion_r1540816389
PR Review Comment: https://git.openjdk.org/lilliput/pull/128#discussion_r1540886764