RFR: 8338526: Don't store abstract and interface Klasses in class metaspace

[Thomas Stuefe]

On Thu, 9 May 2024 13:51:09 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> This change stores InstanceKlass for interface and abstract classes in the non-class metaspace, since class metaspace will have limits on number of classes that can be represented when Lilliput changes go in.  Classes that have no instances created for them don't require compressed class pointers.  The generated LambdaForm classes are also AllStatic, and changing them to abstract moves them to non-class metaspace too.  It's not technically great to make them abstract and not final but you can't have both.  Java classfile access flags have no way of specifying something like AllStatic.
> 
> Tested with tier1-8.

Hi Coleen,

IIUC, the new "is_in_klass_space" function that now is present in all Metadata children only exists because of the template function in MetadataFactory, right? Just for the purpose of deallocation?

If so, see this proposed addition to your patch: https://gist.github.com/tstuefe/5111c735b12f6d9c3c1d32699d0820f6

This would make Metaspace::deallocate smarter - Metaspace knows whether a given pointer is in class space or not, it can do automatically the right thing. There should be no need to tell it how to deallocate that storage. (If you are worried, in debug builds there are also sanity checks).

If you do this, I think you could remove all variants of "is_in_klass_space" apart from the one in Klass.

src/hotspot/share/memory/allocation.hpp line 319:

> 317:   f(SharedClassPathEntry) \
> 318:   f(RecordComponent) \
> 319:   f(AbstractClass)

This is a minor nit: I assume this new constant is just there to steer allocation down in metaspace away from Class space? This breaks a bit with established pattern, because the type `Klass::type()` returns is still "ClassType", so this new constant never really appears anywhere. Its only point is "its not classtype". You could probably hand down any other constant to Metaspace::allocate, as long as its not `ClassType`.

What we should eventually do, but in a follow up RFE:  modify `Metaspace::allocate` to replace the `MetaspaceObj::Type` parameter with a `MetadataType mdType` parameter. Or a plain "allocate_in_classspace_please" boolean parameter. Because Metaspace::allocate does not really need to know the type of the metadata object. It just needs to know if the caller insists on having the data in class space.

 But lets do this in a follow-up.

src/hotspot/share/oops/instanceKlass.cpp line 456:

> 454: 
> 455:   InstanceKlass* ik;
> 456:   MetaspaceObj::Type type = (parser.is_interface() || parser.is_abstract()) ? MetaspaceObj::AbstractClassType : MetaspaceObj::ClassType;

small nit, const or constexpr?

src/hotspot/share/oops/klass.hpp line 205:

> 203: 
> 204:   void* operator new(size_t size, ClassLoaderData* loader_data, size_t word_size, TRAPS) throw();
> 205: 

Oh ArrayKlass never used this? Its good to move it to InstanceKlass.

src/hotspot/share/oops/klass.hpp line 214:

> 212:   virtual bool is_klass() const { return true; }
> 213: 
> 214:   bool is_in_klass_space() const { return !is_interface() && !is_abstract(); }

This name is misleading. As a caller, I expect a function with this name to make a range check of Klass* to be inside class space range. This is more of a "should be".

(We also write class space with `c` throughout hotspot, its weird to have it with `k` now)

How about, instead: "needs_narrow_klass_id" or "must_be_narrow_encodable" or similar? That clearly says what you want, that this class needs to be encodable with a narrow id for whatever is your reason. This leaves room for future changes (e.g. a possible future where we need narrow klass ids for other reasons than to make heap objects smaller, or where there is no class space anymore).

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

PR Review: https://git.openjdk.org/jdk/pull/19157#pullrequestreview-2256433389
PR Review Comment: https://git.openjdk.org/jdk/pull/19157#discussion_r1728452223
PR Review Comment: https://git.openjdk.org/jdk/pull/19157#discussion_r1728421710
PR Review Comment: https://git.openjdk.org/jdk/pull/19157#discussion_r1728421197
PR Review Comment: https://git.openjdk.org/jdk/pull/19157#discussion_r1728417646