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

[Ioi Lam]

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.

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?

Thanks
- Ioi