[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: [RFR] (M) 8143608: Don't 64-bit align start of InstanceKlass vtable, itable, and nonstatic_oopma
From: Mikael Gerdin <mikael.gerdin () oracle ! com>
Date: 2015-12-16 9:01:03
Message-ID: 5671284F.6090808 () oracle ! com
[Download RAW message or body]
Hi Chris,
On 2015-12-16 08:40, Chris Plummer wrote:
> Hi Mikael,
>
> On 12/15/15 1:56 AM, Mikael Gerdin wrote:
>> 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@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@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,
> I've gotten some internal guidance to limit the fixes to just the stuff
> that needs adjusting, and then file bugs for the rest.
Ok.
>> filing follow ups on the extra alignObjectSize and missing extra fields.
> Ok. It turns out the alignObjectSize() is needed. I missed that that
> InstanceKlass::size() also does this, and after some internal
> discussion, it was agreed that it should be kept to make sure that
> InstanceKlass::size() is consistent with the amount of memory actually
> allocated for it (after the allocator does padding out to 8-bytes).
> However, there are some other issues to be addressed in this aligning of
> the size. It should not be using ObjectAlignmentInBytes, but just be
> doing 8-byte aligning. I'll file a bug for that, and have it cover both
> the InstanceKlass and SA work needed for this specific issue. A second
> bug will be filed for the word size scaling issue, not accounting for
> the extra word that interface classes and anonymous classes need, and
> for the lack of any test.
Ok.
/Mikael
>>
>> 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.
> ok.
>
> thanks,
>
> Chris
>>
>> /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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic