CR 8005604: HPROF should report the actual instance size

[Aleksey Shipilev]

Alan,

Good catch. This is the excerpt from the HPROF binary format in
heapDumper.cpp:

HPROF_GC_CLASS_DUMP
    (----snip-----)
    u4         instance size (in bytes)
    (----snip-----)
    u2         number of static fields
    [id,       static field name,
     ty,       type,
     vl]*      and value
    (----snip-----)
    u2         number of inst. fields (not inc. super)
    [id,       instance field name,
     ty]*      type

HPROF_GC_INSTANCE_DUMP
    u4         number of bytes that follow
    [vl]*      instance field values (class, followed
               by super, super's super ...)

Seems like that instance_size() method is used to populate both GC_CLASS
and GC_INSTANCE. Would it be OK to push the VM-reported size to GC_CLASS
only, and leave GC_INSTANCE intact?

-Aleksey.

On 12/29/2012 02:02 PM, Alan Bateman wrote:
> Aleksey,
> 
> I'm a bit out of touch with this code but just to say that the HPROF
> format is VM independent and the length that follows the class ID in the
> INSTANCE record is the size for the fields that follow.
> 
> -Alan
> 
> On 29/12/2012 09:14, Aleksey Shipilev wrote:
>> Hi guys,
>>
>> This issue popped out (again?) in the course of @Contended work, which
>> does the padding around fields in some corner cases. HPROF just iterates
>> the fields to deduce the instance size, which can give a terribly wrong
>> instance size estimate.
>>
>> I have filed CR 8005604 to track this.
>>
>> FWIW, the fix appears to be very simple, we can reuse the instance size
>> data already available in instanceKlass, like this:
>>
>> --- a/src/share/vm/services/heapDumper.cpp    Mon Dec 24 11:46:38 2012
>> -0800
>> +++ b/src/share/vm/services/heapDumper.cpp    Sat Dec 29 13:08:51 2012
>> +0400
>> @@ -772,33 +772,7 @@
>>   u4 DumperSupport::instance_size(Klass* k) {
>>     HandleMark hm;
>>     instanceKlassHandle ikh = instanceKlassHandle(Thread::current(), k);
>> -
>> -  int size = 0;
>> -
>> -  for (FieldStream fld(ikh, false, false); !fld.eos(); fld.next()) {
>> -    if (!fld.access_flags().is_static()) {
>> -      Symbol* sig = fld.signature();
>> -      switch (sig->byte_at(0)) {
>> -        case JVM_SIGNATURE_CLASS   :
>> -        case JVM_SIGNATURE_ARRAY   : size += oopSize; break;
>> -
>> -        case JVM_SIGNATURE_BYTE    :
>> -        case JVM_SIGNATURE_BOOLEAN : size += 1; break;
>> -
>> -        case JVM_SIGNATURE_CHAR    :
>> -        case JVM_SIGNATURE_SHORT   : size += 2; break;
>> -
>> -        case JVM_SIGNATURE_INT     :
>> -        case JVM_SIGNATURE_FLOAT   : size += 4; break;
>> -
>> -        case JVM_SIGNATURE_LONG    :
>> -        case JVM_SIGNATURE_DOUBLE  : size += 8; break;
>> -
>> -        default : ShouldNotReachHere();
>> -      }
>> -    }
>> -  }
>> -  return (u4)size;
>> +  return (u4)ikh->size_helper();
>>   }
>>
>>   // dumps static fields of the given class
>>
>>
>> Please let me know if you have troubles with pushing this. If not, I can
>> get the proper due diligence to get this fixed.
>>
>> Thanks,
>> Aleksey.
>