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

[Mikael Gerdin]

On 2015-12-08 20:14, Chris Plummer wrote:
> [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.

I'd say that it's quite possible to test it!
Create a whitebox.cpp entry point for determining the size of a class.

Run a java program calling the entry point and printing the 
InstanceKlass size as computed by calling InstanceKlass::size() on a set 
of pre-determined set of complex and simple classes (vtables, itables, 
anonymous, etc.)
Have the java program wait after it's finished printing.

Launch the SA agent and attach it to the process.
Through several layers of magic, execute the incantation:

mgerdin at mgerdin03:~/work/repos/hg/jdk9/hs-rt-work/hotspot$ 
../build/linux-x86_64-normal-server-slowdebug/images/jdk/bin/java 
sun.jvm.hotspot.CLHSDB 6211
Attaching to process 6211, please wait...
hsdb> jseval 
"sapkg.utilities.SystemDictionaryHelper.findInstanceKlass('java/lang/Object').getSize();"
472

You could also create a javascript source file in the test directory 
which performs the appropriate calls and do
hsdb> jsload /path/to/file.js
hsdb> jseval "doit()"

where
file.js:
doit = function() {
  sd = sapkg.utilities.SystemDictionaryHelper;
  do_klass = function(name) {
      writeln(sd.findInstanceKlass(name).getSize());
  }

  do_klass('java/lang/Object');
  do_klass('java/lang/Class');
}


The only problem is that the test can't reliably execute on unconfigured 
os x setups because of OS level security requirements for attaching to 
processes.

After detaching HSDB with the "quit" command you tell the debugged java 
process to terminate, for example by printing some string on its stdin.

Easy, right? :)

/Mikael

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