Re: 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