RFR: JDK-8159127: hprof heap dumps broken for lambda classdata

[serguei.spitsyn at oracle.com]

Hi Jini,


It looks good in general.

Some minor comments though.

  http://cr.openjdk.java.net/~jgeorge/8159127/webrev.01/hotspot/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/classfile/ClassLoaderData.java.udiff.html

+ public ClassLoaderData next() {
+ Address classLoaderDataAddr = nextField.getValue(getAddress());
+ if (classLoaderDataAddr == null) {
+ return null;
+ }
+
+ return instantiateWrapperFor(classLoaderDataAddr);
+ }
+
+ public Klass getKlasses() {
+ Address klassesFieldAddr = klassesField.getValue(getAddress());
+ if (klassesFieldAddr == null) {
+ return null;
+ }
+ return (InstanceKlass)Metadata.instantiateWrapperFor(klassesFieldAddr);
+ }


   It seems there is no need to check address for null as it is done 
inside the instantiateWrapperFor call.

ClassLoaderData.java:

private static VirtualBaseConstructor<Metadata> metadataConstructor;
. . .

public static Metadata instantiateWrapperFor(Address addr) {
   return metadataConstructor.instantiateWrapperFor(addr);
}

VirtualBaseConstructor.java:
public T instantiateWrapperFor(Address addr)throws WrongTypeException {
   if (addr ==null) {
     return null;
   }
   . . .



http://cr.openjdk.java.net/~jgeorge/8159127/webrev.01/hotspot/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java.frames.html

At the lines 522-547 it is possible to define and use the same callback class
for traversing both the SystemDictionary classes and the anonymous classes.
The same is applied to the lines 961-990.
But I leave it up to you to decide if it needs to be updated.

Thanks, Serguei On 12/13/16 08:34, Jini Susan George wrote:
> Thank you, Dmitry. Could I get one more reviewer to take a look at this ?
>
> -Jini.
>
>> -----Original Message-----
>> From: Dmitry Samersoff
>> Sent: Tuesday, December 13, 2016 2:48 PM
>> To: Jini Susan George; serviceability-dev at openjdk.java.net
>> Subject: Re: RFR: JDK-8159127: hprof heap dumps broken for lambda
>> classdata
>>
>> Jini,
>>
>> Looks good to me.
>>
>> -Dmitry
>>
>> On 2016-12-13 11:55, Jini Susan George wrote:
>>> Modified webrev:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8159127/webrev.01/
>>>
>>> Thanks,
>>> Jini.
>>>
>>>> -----Original Message-----
>>>> From: Jini Susan George
>>>> Sent: Tuesday, December 13, 2016 10:09 AM
>>>> To: Dmitry Samersoff; serviceability-dev at openjdk.java.net
>>>> Subject: RE: RFR: JDK-8159127: hprof heap dumps broken for lambda
>>>> classdata
>>>>
>>>> Thank you, Dmitry. I will add the null check.
>>>> -jini
>>>>
>>>>> -----Original Message-----
>>>>> From: Dmitry Samersoff
>>>>> Sent: Monday, December 12, 2016 5:06 PM
>>>>> To: Jini Susan George; serviceability-dev at openjdk.java.net
>>>>> Subject: Re: RFR: JDK-8159127: hprof heap dumps broken for lambda
>>>>> classdata
>>>>>
>>>>> Jini,
>>>>>
>>>>> Looks good to me!
>>>>>
>>>>> ClassLoaderData.java
>>>>>
>>>>> 85: Should we check klassField for null here?
>>>>>
>>>>> -Dmitry
>>>>>
>>>>> On 2016-12-12 13:31, Jini Susan George wrote:
>>>>>> Could I please get a review done for the following SA defect?
>>>>>>
>>>>>>
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8159127
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~jgeorge/8159127/webrev.00/
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> - Jini Susan George
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Dmitry Samersoff
>>>>> Oracle Java development team, Saint Petersburg, Russia
>>>>> * I would love to change the world, but they won't give me the sources.
>>
>> --
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.