On Tue, 16 Dec 2025 12:26:42 GMT, Stefan Karlsson <[email protected]> wrote:

> The following function in `InstanceKlass` is supposed to return size in words:
> 
>   static int size(int vtable_length, int itable_length,
>                   int nonstatic_oop_map_size,
>                   bool is_interface,
>                   bool is_inline_type) {
>     return align_metadata_size(header_size() +
>            vtable_length +
>            itable_length +
>            nonstatic_oop_map_size +
>            (is_interface ? (int)sizeof(Klass*)/wordSize : 0) +
>            (is_inline_type ? (int)sizeof(InlineKlassFixedBlock) : 0));
>   }
> 
> but `sizeof(InlineKlassFixedBlock)` returns a size in bytes. This adds 640 
> bytes instead of 80 bytes to all InlineKlasses.
> 
> This can be seen by running `jcmd <pid> VM.classes` to check the size of the 
> InlineKlass of Integer before and after the fix for this:
> 
> Before:
> 
> 0x00000ffc0024d260 167 fully_initialized WS java.lang.Integer
> 
> After:
> 
> 0x000001800024d260 97 fully_initialized WS java.lang.Integer
> 
> 
> That is in words. So this bug added an extra `(167 - 97) * 8 == 560` bytes. 
> 
> I've so far only done some local smoke testing with TEST=hotspot_valhalla

This is a good catch! I think it would be good to stress test these changes 
with (App)CDS/AOT and COH on/off, possibly in conjunction. Judging by the fact 
that the RefArrayKlass assertion was kind of weak this whole time, I wouldn't 
be surprised if this unearths more CDS issues.

src/hotspot/share/oops/flatArrayKlass.hpp line 95:

> 93: 
> 94:   // sizing
> 95:   static int header_size()  { return sizeof(FlatArrayKlass) / wordSize; }

Curiously enough we actually got this right for `RefArrayKlass::header_size()` 
and I'm guessing the following assertion didn't actually do anything meaningful:

assert(RefArrayKlass::header_size() <= InstanceKlass::header_size(),
         "array klasses must be same size as InstanceKlass");

src/hotspot/share/oops/instanceKlass.hpp line 1050:

> 1048:            nonstatic_oop_map_size +
> 1049:            (is_interface ? (int)sizeof(Klass*) / wordSize : 0) +
> 1050:            (is_inline_type ? (int)sizeof(InlineKlassFixedBlock) / 
> wordSize : 0));

Do we have an `x / wordSize` helper function anywhere? Too much inline 
arithmetic makes it hard to maintain, imo.

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

Marked as reviewed by phubner (Committer).

PR Review: 
https://git.openjdk.org/valhalla/pull/1804#pullrequestreview-3582934819
PR Review Comment: 
https://git.openjdk.org/valhalla/pull/1804#discussion_r2623156322
PR Review Comment: 
https://git.openjdk.org/valhalla/pull/1804#discussion_r2623158498

Reply via email to