RFR(XS) 8230674 Heap dumps should exclude dormant CDS archived objects of unloaded classes

[Ioi Lam]

On 9/5/19 11:11 PM, David Holmes wrote:
> On 6/09/2019 1:39 pm, Ioi Lam wrote:
>> On 9/5/19 8:18 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 6/09/2019 12:27 pm, Ioi Lam wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8230674
>>>> http://cr.openjdk.java.net/~iklam/jdk14/8230674-heap-dump-exclude-dormant-oops.v01 
>>>>
>>>>
>>>> Please review this small fix:
>>>>
>>>> When CDS is in use, archived objects are memory-mapped into the 
>>>> heap (currently G1GC only). These objects are partitioned into
>>>> "subgraphs". Some of these subgraphs may not be loaded (e.g., those
>>>> related to jdk.internal.math.FDBigInteger) at the time a heap dump is
>>>> requested. >
>>>> When a subgraph is not loaded, some of the objects in this subgraph 
>>>> may belong to a class that's not yet loaded.
>>>>
>>>> The bug happens when such an "dormant" object is dumped, but its class
>>>> is not dumped because the class is not in the system dictionary.
>>>>
>>>> There is already code in DumperSupport::dump_instance() that tries 
>>>> to handle dormant objects, but it needs to be extended to cover 
>>>> arrays, as well as and references from non-dormant object/arrays to 
>>>> dormant ones.
>>>
>>> I have to confess I did not pay any attention to the CDS archived 
>>> objects work, so I don't have a firm grasp of how you have 
>>> implemented things. But I'm wondering how can you have a reference 
>>> to a dormant object from a non-dormant one? Shouldn't the act of 
>>> becoming non-dormant automatically cause the subgraph from that 
>>> object to also become non-dormant? Or do you have "read barriers" to 
>>> perform the changes on demand?
>>>

Ah -- my bug title is not correct.

I changed the bug title (and this e-mail subject) to

Heap dumps should exclude dormant CDS archived objects **of unloaded 
classes**

During the heap dump, we scan all objects in the heap, regardless of 
reachability. There's no way to decide reachability in 
HeapObjectDumper::do_object(), unless we perform an actual GC.

But it's OK to include unreachable objects in the heap dump. (I guess 
it's useful to see how much garbage you have in the heap. There's an 
option to run a collection before dumping the heap.)

There are 2 kinds of unreachable objects -- garbage: those that were 
once reachable but no longer, dormant: the archived objects that have 
never been reachable.

Anyway, it's OK to dump dormant objects as long as their class has been 
loaded. The problem happens only when we dump a dormant object who class 
is not yet loaded (Eclipase MAT get confused when it sees an object 
whose class ID is invalid).

So to answer your question, we can have a case with a dormant array 
(that contains a dormant object) like this:

     Object[] array = {new ClassNotYetLoaded();}

After my fix, the array will be dumped (we have no easy way of not doing 
that), but its contents becomes this in the .hprof file:

     Object[] array = {null}

Thanks
- Ioi



>>
>> Hi David,
>>
>> Thanks for the review.
>>
>> The dormant objects are not reachable via the GC roots. They become 
>> non-dormant via explicit calls to JVM_InitializeFromArchive, after 
>> which they become reachable via the static fields of loaded classes.
>
> Right, so is there a distinction between non-dormant and reachable at 
> the time an object becomes non-dormant? I'm still unclear how a drmant 
> array becomes non-dormant but still contains elements that refer to 
> dormant objects.
>
>> The only issue here is heap dump is done by scanning all objects in 
>> the heap, including unreachable ones
>>
>>    HeapObjectDumper obj_dumper(this, writer());
>>    Universe::heap()->safe_object_iterate(&obj_dumper);
>>
>> that's how these dormant objects are discovered during heap dump.
>>
>>> That aside the code changes seem reasonable, you moved the check out 
>>> of DumperSupport::dump_instance and into the higher-level 
>>> HeapObjectDumper::do_object so that it catches instances and arrays, 
>>> plus you added a check for array elements.
>>>
>>
>> I am debating whether I should put the masking code in here:
>>
>> void DumpWriter::write_objectID(oop o) {
>>    o = mask_dormant_archived_object(o);  /// <---- add
>>    address a = (address)o;
>> #ifdef _LP64
>>    write_u8((u8)a);
>> #else
>>    write_u4((u4)a);
>> #endif
>> }
>>
>>
>> That way, even if a dormant object (unintentionally) becomes 
>> reachable via the GC roots, we won't write an invalid reference to it 
>> (the object "body" will not be written, so the ID will not point to 
>> anything valid).
>>
>> But this seems a little too aggressive to me. What do you think?
>
> It does seem a little aggressive as it seems to introduce the dormancy 
> check into a lot of places that don't need it. But as I said I don't 
> know this code so I'm really not the right person to ask.
>
> Cheers,
> David
> -----
>
>> Thanks
>> - Ioi
>>