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

[David Holmes]

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