[RFR] (M) 8143608: Don't 64-bit align start of InstanceKlass vtable, itable, and nonstatic_oopmap on 32-bit systems
chris.plummer at oracle.com
Fri Jan 15 22:27:54 UTC 2016
On 1/15/16 2:12 PM, Coleen Phillimore wrote:
> 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:
>> 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:
>> 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
>> 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.
>> On 12/4/15 2:02 PM, Chris Plummer wrote:
>>> Please review the following:
>>> 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.
More information about the hotspot-runtime-dev