On Fri, 23 Aug 2024 20:37:55 GMT, Coleen Phillimore <[email protected]> wrote:
>> 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.
I have to think about this more.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19157#discussion_r1729492034
