RFR: JDK-8134030: test/serviceability/dcmd/gc/HeapDumpTest fails to verify the dump

[Andreas Eriksson]

On 2015-11-04 08:31, David Holmes wrote:
> On 3/11/2015 9:10 PM, Andreas Eriksson wrote:
>>
>>
>> On 2015-11-03 07:27, David Holmes wrote:
>>> Hi Andreas,
>>>
>>> On 31/10/2015 12:03 AM, Andreas Eriksson wrote:
>>>> Hi,
>>>>
>>>> Please review this change to heap dump logic.
>>>> Including runtime list, since this is related to class loading.
>>>>
>>>> Bug:
>>>> JDK-8134030: test/serviceability/dcmd/gc/HeapDumpTest fails to verify
>>>> the dump
>>>> https://bugs.openjdk.java.net/browse/JDK-8134030
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~aeriksso/8134030/webrev.00/
>>>>
>>>> A heap dump can occur at a point where an InstanceKlass doesn't have a
>>>> Java class mirror yet.
>>>> To avoid a crash because of this JDK-8131600 [1] excluded classes that
>>>> are not linked yet from the dump.
>>>>
>>>> The problem here is that since a class that has been loaded is already
>>>> exposed to the Java side, another class can have a reference to the
>>>> excluded class.
>>>> So, if it has been *loaded* but not *linked* it will be excluded, but
>>>> can still be referenced by other classes in the dump.
>>>
>>> I'm having trouble seeing exactly when the class is marked as "loaded"
>>> but it certainly seems to be after the mirror has been set. However
>>> linking happens even later than that - so if you still see a crash
>>> excluding linked classes then I fear you will also see problems
>>> excluding loaded classes!
>>>
>>> David
>>> -----
>>>
>>
>> Hi,
>>
>> The crash does not happen when excluding linked classes, sorry if that
>> was unclear.
>>
>> Let me try to explain it again:
>> The crash was fixed by excluding non-linked classes (in JDK-8131600).
>> But that excludes some classes that are already referenced in the heap
>> dump by other classes.
>> By changing to excluding non-loaded classes, the crash can still not
>> happen, and all classes that are referenced in the dump are also 
>> included.
>
> Thanks for clarifying, I had misunderstood.
>
> Change seems fine in that case.
>

Thanks.

- Andreas

> Thanks,
> David
>
>
>> Not dumping these classes is only a problem when we try to look at the
>> heap dump at a later point (and even then it is only a problem of
>> missing information, not a crash).
>> For example, HeapDumpTest gives the following warning when it tries to
>> follow a reference to a class that was excluded from the dump: WARNING:
>> Failed to resolve object id 0x6c003d348 [...].
>> VisualVM on the other hand will ignore the value, and tell the user the
>> field was null (which is very misleading).
>>
>> I hope that made sense.
>>
>> Thanks,
>> Andreas
>>
>>>
>>>> This gives the following warning in HeapDumpTest:
>>>> WARNING: Failed to resolve object id 0x6c003d348 [...].
>>>>
>>>> This fix changes heapDumper.cpp to only exclude classes that are 
>>>> not yet
>>>> loaded instead.
>>>>
>>>> I'd like confirmation on this from someone who knows more about loaded
>>>> vs linked, but I think that this is correct:
>>>> When a class has been loaded, we are guaranteed that a Java class 
>>>> mirror
>>>> is setup, so the crash seen in JDK-8131600 still cannot happen.
>>>> This seems to be confirmed by the manual reproducer from JDK-8131600,
>>>> which still succeeds with this fix.
>>>>
>>>> Also in this change is the re-addition of the actual heap dump
>>>> verification call in HeapDumpTest, which was accidentally removed when
>>>> the test was re-factored.
>>>>
>>>> Regards,
>>>> Andreas
>>>>
>>>> [1]: https://bugs.openjdk.java.net/browse/JDK-8131600
>>