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

[David Holmes]

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,
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
>