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

Fri Jan 22 16:49:18 UTC 2016

On 1/22/16 11:08 AM, Chris Plummer wrote:
> On 1/18/16 5:21 AM, Mikael Gerdin wrote:
>> On 2016-01-18 10:41, Mikael Gerdin wrote:
>>> Hi,
>>>
>>> On 2016-01-15 23:12, 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.
>>>>
>>>>
>>>> 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/ 
>>>>>
>>>
>>> In sharkTopLevelBlock.cpp there is a bit of code to perform the
>>> alignment which you have removed from the other platforms, search for
>>> "needs_aligning". I'm not sure if shark even builds but it might be
>>> courteous to at least attempt to update the sources.
>>>
>>> It's interesting that PPC64 does not appear to need any change, but I
>>> don't see any code to align up after loading the vtable length in the
>>> PPC sources.
>>
>> I've just realized that this fix only changes the behavior on 32 bit 
>> platforms, so PPC64 never needed it.
>>
> Hi Mikael,
>
> Sorry, I missed your responses since I thought I was always getting cc'd.
>
> You are correct that on 64-bit there is effectively no change. 
> align_object_size() is a nop for 64-bit. My changes do include a diff 
> for AARCH64, but it is to remove what is effectively dead code. The 
> code is doing 64-bit alignment when the word size is 32-bit, but of 
> course this never happens for AARCH64. I believe it was copied from 
> the x86 port to handle the 64-bit alignment on 32-bit archs.

align_object_offset is 64 bits and a nop on 64 bit platforms. 
align_object_size uses ObjectAlignmentInBytes - I will have an RFR to 
fix this based on Chris's change once the repositories are not frozen.

Coleen

>
> thanks,
>
> Chris
>> /Mikael
>>
>>>
>>> Otherwise the change looks good as far as my limited knowledge of this
>>> can tell.
>>>
>>> /Mikael
>>>
>>>
>>>>>
>>>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>