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

[Ioi Lam]

On 9/6/19 4:06 PM, Jiangli Zhou wrote:
> On Fri, Sep 6, 2019 at 3:17 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>> On 9/6/19 11:48 AM, Jiangli Zhou wrote:
>>> On Fri, Sep 6, 2019 at 9:43 AM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>
>>>> 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.
>>> Currently Java object archiving framework only supports one
>>> directional state change: dormant -> live. An archived object can
>>> become a live object from dormant state, but it cannot go back to the
>>> dormant state. Need to investigate thoroughly for all cases before the
>>> 'live -> dormant' transition can be supported. All objects in the
>>> 'Open' archive heap region are associated with the builtin class
>>> loaders and their classes are not unloaded. The existing static fields
>>> for archiving within the JDK classes are selected and the associated
>>> objects do not become garbage once 'installed'.
>>>
>>>> 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).
>>> Yes. That's a scenario needs to be handled for a tool that iterates
>>> the Java heap. A dormant object in the 'Open' archive heap region may
>>> have a 'invalid' klass since the klass may not be loaded yet at the
>>> moment.
>>>
>>> Your webrev looks reasonable to me on high level pending information
>>> for following questions. Can you please give more details on the
>>> dormant objects referenced from the arrays? What specific arrays are
>>> those?
>> Hi Jiangli,
>>
>> Thanks for the review. I add the following code:
>>
>>     // [id]* elements
>>     for (int index = 0; index < length; index++) {
>>       oop o = array->obj_at(index);
>>   >>
>>       if (o != NULL && mask_dormant_archived_object(o) == NULL) {
>>         ResourceMark rm;
>>         tty->print_cr("%s array contains %s object",
>> array->klass()->external_name(), o->klass()->external_name());
>>       }
>>   >>
>>       o = mask_dormant_archived_object(o);
>>       writer->write_objectID(o);
>>     }
>>
>> and the output is:
>>
>> $ java -cp .  -XX:+HeapDumpAfterFullGC HelloGC
>> Dumping heap to java_pid20956.hprof ...
>> [Ljava.lang.Object; array contains java.util.jar.Attributes$Name object
>> [Ljava.lang.Object; array contains java.util.jar.Attributes$Name object
>> (repeated about 20 times)
>>
>> It comes from java/util/jar/Attributes$Name::KNOWN_NAMES. This class is
>> not loaded because my program doesn't use JAR files in the classpath:
> The above looks right and is expected. At this point, we would not see
> any archive object that first becomes live then becomes dormant again.
>
> That however will change in the future when we make the object
> archiving framework general enough for other JDK and application class
> usages. We need to work out the GC details for the live -> dormant
> transition when that happens.

Hi Jiangli,

Sure, we should look into that when we expand the scope of object archiving.

I updated the webrev to log the skipped objects with -Xlog:cds+heap=debug:

http://cr.openjdk.java.net/~iklam/jdk14/8230674-heap-dump-exclude-dormant-oops.v02/

Thanks
- Ioi


> Thanks,
> Jiangli
>
>> Thanks
>> - Ioi
>>
>>
>>> Regards,
>>> Jiangli
>>>
>>>
>>>
>>>> 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
>>>>>>