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

Mikael Gerdin mikael.gerdin at oracle.com
Mon Jan 18 13:21:37 UTC 2016


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.

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



More information about the hotspot-runtime-dev mailing list