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

[Chris Plummer]

After some discussion offline, it was decided that asserts are not 
needed since all the values for which alignment could be checked are 
already scaled to word size. Since we want word size alignment, there's 
no finer alignment that can be checked for.

Chris

On 1/18/16 12:38 PM, Chris Plummer wrote:
> Ok, I'll add some asserts.
>
> Chris
>
> On 1/17/16 7:54 PM, David Holmes wrote:
>> Hi Chris,
>>
>> I'm also okay with the changes, but as Karen asked the other day 
>> (TOI), are there any asserts checking that everything is actually 
>> aligned as expected?
>>
>> Thanks,
>> David
>>
>> On 16/01/2016 8:27 AM, Chris Plummer wrote:
>>> 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
>>>>>>
>>>>>
>>>>
>>>
>