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

[Chris Plummer]

[Adding serviceability-dev at openjdk.java.net]

Hi Mikael,

Thanks for pointing this out. I'll look into it some more. Are there any 
tests that should be failing as a result of this? I get the feeling no, 
since I see other issues here that existed before my change. For 
example, this code is not returning the proper size if the class is 
anonymous or is an interface. It needs to add 1 extra word in that case. 
See size() in instanceKlass.hpp.

Another difference from the VM code is alignObjectSize() is being used 
by getSize(), but headerSize is set using alignObjectOffset(). The VM 
code uses align_object_offset in both cases.

   // Align the object size.
   public static long alignObjectSize(long size) {
     return VM.getVM().alignUp(size, 
VM.getVM().getMinObjAlignmentInBytes());
   }

   // All vm's align longs, so pad out certain offsets.
   public static long alignObjectOffset(long offset) {
     return VM.getVM().alignUp(offset, VM.getVM().getBytesPerLong());
   }

So the difference here is in the use of getMinObjAlignmentInBytes (not 
what the VM does) vs getBytesPerLong (what the VM uses):

   public int getObjectAlignmentInBytes() {
     if (objectAlignmentInBytes == 0) {
         Flag flag = getCommandLineFlag("ObjectAlignmentInBytes");
         objectAlignmentInBytes = (flag == null) ? 8 : (int)flag.getIntx();
     }
     return objectAlignmentInBytes;
   }

So this seems wrong for use in any InstanceKlass size or embedded field 
offset calculation. It is probably a remnant of when class metadata was 
stored in the java heap, and the size of InstanceKlass had to be padded 
out to the minimum heap object alignment. At least it is harmless if 
ObjectAlignmentInBytes is not set, and if set it is only supported for 
64-bit:

   lp64_product(intx, ObjectAlignmentInBytes, 
8,                             \
           "Default object alignment in bytes, 8 is 
minimum")                \
           range(8, 
256)                                                     \
constraint(ObjectAlignmentInBytesConstraintFunc,AtParse) \

I'll get these cleaned up, but it sure would be nice if there was a way 
to reliably test it.

thanks,

Chris

On 12/8/15 1:54 AM, Mikael Gerdin wrote:
> Hi Chris,
>
> Not a review but I'm fairly sure that you need to update the 
> serviceability agent to reflect these changes, see for example:
>
> public long getSize() {
>   return Oop.alignObjectSize(
> getHeaderSize() +
> Oop.alignObjectOffset(getVtableLen()) +
> Oop.alignObjectOffset(getItableLen()) + 
> Oop.alignObjectOffset(getNonstaticOopMapSize()));
>   }
>
> in InstanceKlass.java
>
> /Mikael
>
> On 2015-12-04 23:02, 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
>>
>