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

Chris Plummer chris.plummer at oracle.com
Mon Jan 18 20:38:02 UTC 2016


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



More information about the hotspot-runtime-dev mailing list