Chris,
1. Please file a separate CR for SA (assign it to me) and go ahead with
your fix when it become ready.
2. As far as I understand the fix, you just removed explicit alignment
for all platforms (not only 32bit) and rely on compiler. I'm a bit
scared with it (see David's comments).
Also please contact SQE (Leonid Mesnik) about testing of the fix before
pushing it.
-Dmitry
On 2015-12-11 00: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"
>>
>> 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.
>
> 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).
>
> 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
>>>>>
>>>>
>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the source code.
