[RFR] (M) 8143608: Don't 64-bit align start of InstanceKlass vtable, itable, and nonstatic_oopmap on 32-bit systems

[David Holmes]

Hi Chris,

On 5/12/2015 8:02 AM, Chris Plummer wrote:
> Hello,
>
> Please review the following:
>
> https://bugs.openjdk.java.net/browse/JDK-8143608
> http://cr.openjdk.java.net/~cjplummer/8143608/webrev.00/webrev.hotspot/
>
> A bit of background would help. The InstanceKlass object has a number of
> variable length fields that are laid out after the declared fields. When
> an InstanceKlass object is allocated, extra memory is allocated for it
> to leave room for these fields. The first three of these fields are
> vtable, itable, and nonstatic_oopmap. They are all arrays of HeapWord
> sized values, which means void* size, which means they only need 32-bit
> alignment on 32-bit systems. However, they have always been 64-bit
> aligned. This webrev removes the forced 64-bit alignment on 32-bit
> systems, saving footprint.

Theory sounds fine.

Not so sure about the practice though. In instanceKlass.hpp you have 
removed a lot of alignment adjustments and I can't be sure they are all 
necessarily correct. In particular:

934   static int header_size()            { return 
sizeof(InstanceKlass)/HeapWordSize; }

seems to me this will truncate the header size if sizeof(InstanceKlass) 
is not already 32-bit aligned. Are we assuming the compiler will always 
pad things such that this is true?

In general I would have expected all the align_object_offset (which 
aligns to 64-bit) to be replaced with something that aligns to 
sizeof(void*). Even if we can assume a minimum of 32-bit alignment it 
isn't clear to me that that always converts to 64-bit alignment on 64-bit.

Thanks,
David

> This change affects all 32-bit platforms. It should have no net impact
> on 64-bit platforms since the fields remain (naturally) 64-bit aligned
> (unless of course I've introduced a bug). The intent is to not change
> what is done for 64-bit platforms.
>
> BTW, there is a change to AARCH64, which may seem odd at first. It just
> removes an "if" block where the condition should always have evaluated
> to false, so it should have no net affect.
>
> Tested with JPRT "-testset hotspot". Please let me know if you think
> there are any additional tests that should be run.
>
> thanks,
>
> Chris
>