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

[Coleen Phillimore]

Hi,

I've read this change and it looks correct to me.  The 32/64 bit natural 
alignment should be used for the vtable and itable embedded fields, and 
this change accomplishes this.


On 12/21/15 3:32 PM, Chris Plummer wrote:
> Hello,
>
> I'm doing a bit of a reset on this thread. I've updated the webrev. 
> The only change is fixing SA to also no longer do the aligning:
>
> https://bugs.openjdk.java.net/browse/JDK-8143608
> http://cr.openjdk.java.net/~cjplummer/8143608/webrev.03/webrev.hotspot/
>
> I've also filed the following CRs for pre-existing bugs instead of 
> addressing them with these changes:
>
> https://bugs.openjdk.java.net/browse/JDK-8145627 -
>     sun.jvm.hotspot.oops.InstanceKlass::getSize() returns the 
> incorrect size and has no test
> https://bugs.openjdk.java.net/browse/JDK-8145628
> hotspot metadata classes shouldn't use HeapWordSize or heap related 
> macros like align_object_size

I am currently working on this bug because it is wrong to use 
align_object_size() for metadata.  Metadata should be naturally aligned 
and I'll verify it's the case in metaspace allocation code.

Thanks to Mikael for the alert about SA code that copies this.  It seems 
that the SA shouldn't report metadata sizes if that's also reported with 
jcmd.  Maybe the jstat -classstats functionality that uses SA (which is 
wrong now) can be removed?  And why would someone use this on a core file?

>
> And just restating here why I think the removal of these 
> align_object_field references is safe:
>
> The argument to align_object_offset() is the offset in words, and it 
> returns the offset in words after aligning to a HeapWordsPerLong 
> boundary. Since the argument is in words, there is no concern with my 
> changes removing word alignment of something that is not already word 
> aligned. It must already be word aligned or the current implementation 
> would be broken.
>
> Since HeapWordsPerLong is 1 on 64-bit, the end result is that 
> align_object_offset() is a no-op, so the removal is having no impact 
> on 64-bit.

It (align_object_offset) is actually wrong, but since the size of the 
objects are naturally aligned, I think it's better to not have some 
explicit alignment call.
>
> On 32-bit HeapWordsPerLong is 2, so the end result is that the offset 
> will remain 32-bit word aligned, and not be 64-bit aligned. This is in 
> fact the goal of this change, so what remains is to show that 64-bit 
> alignment of the fields is not needed on 32-bit. Since all of the 
> fields for which alignment has been removed are 32-bit types, it is 
> safe to keep them 32-bit word aligned instead of making them 64-bit 
> aligned.
>

Agree.  This looks good.

Thanks,
Coleen

> thanks,
>
> Chris
>
>
>
> On 12/4/15 2:02 PM, 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.
>>
>> 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
>>
>