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

[Chris Plummer]

Hi Coleen,

On 1/15/16 2:12 PM, Coleen Phillimore wrote:
>
> 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.
Thanks for looking at this. It's much appreciated.
>
>
> 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?
Well, that's a good question for the serviceability team.
>
>>
>> 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.
Great. Thanks again.

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