[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
Tue Dec 15 09:56:31 UTC 2015
Hi Chris,
sorry for the late reply.
On 2015-12-10 22:31, Chris Plummer wrote:
> Hi Mikael,
>
> See comments inline below:
>
> On 12/9/15 8:48 AM, Mikael Gerdin wrote:
>> 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.
> Ok, but I instead decided to use jcmd with "GC.class_stats KlassBytes"
Sounds good.
>>
>> 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.)
> For now I just did this from the command line to do some quick checking.
> No test written.
>> 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
> Ok. I did this to get sizes as SA sees them. They were not just wrong
> for existing JDK, but in most cases off by a large margin. I did some
> investigating. This is InstanceKlass.getSize()
>
> public long getSize() {
> return Oop.alignObjectSize(getHeaderSize() +
> Oop.alignObjectOffset(getVtableLen()) +
> Oop.alignObjectOffset(getItableLen()) +
> Oop.alignObjectOffset(getNonstaticOopMapSize()));
> }
>
> The problem is that getHeaderSize() returns the size in bytes, but the
> others all return the size in words. They need to be multiplied by the
> word size to get the right value since the caller of getSize() expects
> the result to be in bytes.
Oh, that seems like a familiar problem :)
>
> So we have multiple problems with SA with respect to the
> InstanceKlass.getSize() support:
>
> * Alignment issues introduced by me.
> * Some minor other issues like using alignObjectSize when it's not
> needed, and not taking into account extra fields for interfaces and
> anonymous classes.
> * Not multiplying the vtable, itable, and oopMapSize by the number of
> bytes in a word.
> * No test available.
>
> I'm not too sure how to proceed here w.r.t. my CR. I'm inclined to just
> make the SA adjustments needed to take into consideration the alignment
> changes I've made to InstanceKlass, and file a CRs for the rest (one for
> the existing bugs and one for the lack of any test).
It would be good if you could at least fix the obvious word size scaling
bug, filing follow ups on the extra alignObjectSize and missing extra
fields.
The main consumer of this is the "jmap -clstats" command which is a
supported external interface for querying the sizes of metadata in
relation to their ClassLoaders.
/Mikael
>
> Please advise.
>
> thanks,
>
> Chris
>
>>
>> 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
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list