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

[Coleen Phillimore]

On Fri, 23 Aug 2024 06:43:24 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Incorporated a set of Thomas Stuefe's comments. Take out AbstractClass MetaspaceObj::Type.
>
> 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.

Yes, this is a bit inconsistent.  I like your suggestion for an improvement.  I was going to try to do this in this patch but the MetaspaceObj::Type is used for the report_metadata_oom event, so it's still needed in metaspace::allocate.  Which is unfortunate because I always get these two MetadataType and MetaspaceObj::Type things confused.

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

I changed this line now (and used const for bool).

> 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).

I renamed this is_in_class_space() with the lower case 'c'.  It's still directing metaspace or indicating where the object was allocated.  Your name is a little better but I think not enough until we want to expand the things we want allocated in the class space.  As we talked about, with Tiny Class Pointers, class space will have different things in it (not that these new things need a compressed pointers).  But I think we're better off having less things in the space where their pointers can be compressed since this space is constrained.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19157#discussion_r1729487783
PR Review Comment: https://git.openjdk.org/jdk/pull/19157#discussion_r1729488157
PR Review Comment: https://git.openjdk.org/jdk/pull/19157#discussion_r1729490999