Request for review 8000662: NPG: nashorn ant clean test262 out-of-memory with Java heap

[Coleen Phillimore]

Thanks again for the code review, Stefan.

On 11/29/2012 07:12 AM, Stefan Karlsson wrote:
> Hi Coleen,
>
> I took a closer look at the ClassLoaderData::anonymous_class_loader_data:
>
> I'm wondering if we have a potential bug in that code. Here's a some 
> pseudo code for that:
>  cld = new ClassLoaderData(loader)
>  put_on_cld_list(cld) // GC can now find it
>  init_dependencies // GC can occur at this line
>  cld->set_keep_alive(true) // We mark that this CLD should be 
> considered live by the GC
>
> I think the set_keep_alive call should be done before 
> init_dependencies are set up. Otherwise this CLD might be prematurely 
> unloaded.
>

Yes, that was a bug.   I moved keep_alive setting into 
ClassLoaderDataGraph::add.

>
> Some minor comments:
>
> http://cr.openjdk.java.net/~coleenp/8000662_3/src/os_cpu/linux_x86/vm/os_linux_x86.cpp.patch 
>
>
> Unrelated whitespace change.
>

Reverted file.

>
> http://cr.openjdk.java.net/~coleenp/8000662_3/src/share/vm/classfile/classLoaderData.cpp.udiff.html 
>
>
> is_alive isn't used anymore:
> + void ClassLoaderData::unload(BoolObjectClosure* is_alive) {

You're right.

>
> I think we are using the wrong abstraction when we are looking at the 
> claimed bits here. I'll revisit this code after this has been pushed.
> -    if (data->class_loader() == NULL || 
> is_alive->do_object_b(data->class_loader())) {
> -      assert(data->claimed(), "class loader data must have been 
> claimed");
> +    if (data->claimed() || data->keep_alive()) {
> +      assert(data->class_loader() == NULL || 
> is_alive->do_object_b(data->class_loader()), "class loader data must 
> be live");

Okay, I rewrote this in terms of is_alive closure rather than checking 
the "claimed" bit.   It's asserted to be equivalent to the claimed bit 
except you might change the meaning of claimed for GC someday.

bool ClassLoaderData::is_alive(BoolObjectClosure* is_alive_closure) const {
   bool alive =
     is_anonymous() ?
        is_alive_closure->do_object_b(_klasses->java_mirror()) :
        class_loader() == NULL || 
is_alive_closure->do_object_b(class_loader());
   assert(!alive || claimed(), "must be claimed");
   return alive;
}


And ClassLoaderDataGraph::do_unloading() does:

   while (data != NULL) {
     if (data->keep_alive() || data->is_alive(is_alive_closure)) {
       ... continue ...

>
> http://cr.openjdk.java.net/~coleenp/8000662_3/src/share/vm/classfile/classLoaderData.hpp.patch 
>
>
> Unnecessary change:
>
>    bool is_unloading() const     {
> -    assert(!(is_the_null_class_loader_data() && _unloading), "The 
> null class loader can never be unloaded");
> +    assert(!(this == the_null_class_loader_data() && _unloading), 
> "The null class loader can never be unloaded");
>      return _unloading;
>    }

Fixed.

>
>
> http://cr.openjdk.java.net/~coleenp/8000662_3/src/share/vm/classfile/systemDictionary.cpp.patch 
>
>
> I don't know if this is needed or not. I thought we would have 
> registered some sort of object in the resolved_references to keep the 
> mirror alive. But I guess that's not the case.
>
>        (*appendix_result) = Handle(THREAD, appendix);
> +      // the target is stored in the cpCache and if a reference to this
> +      // MethodName is dropped we need a way to make sure the
> +      // class_loader containing this method is kept alive.
> +      ClassLoaderData* this_key = 
> InstanceKlass::cast(accessing_klass())->class_loader_data();
> +      this_key->record_dependency(m->method_holder(), CHECK_NULL); // 
> Can throw OOM
>        return methodHandle(THREAD, m);
>      }

This would be nice if it's true.  I don't really have a way of proving 
it though and JSR 292 keeps changing so it might not stay true.   I'll 
add a FIXME comment to this though.

Coleen

>
> StefanK
>
> On 11/29/2012 01:43 AM, Coleen Phillimore wrote:
>>
>> Please review updated changes to fix the nashorn OOMs from using jsr 
>> 292 anonymous classes.   The changes to fix bug 8003720 NPG: Method 
>> in interpreter stack frame can be deallocated 
>> <https://jbs.oracle.com/bugs/browse/JDK-8003720>  have been removed 
>> from this changeset in favor of the change that Stefan checked in on 
>> Monday.   Note - there is no target-specific code in this change now.
>>
>> Other changes since last webrev are several cleanups from internal 
>> discussions.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8000662_3/
>> bug link at http://bugs.sun.com/view_bug.do?bug_id=8000662_3
>>
>> Retesting with NSK vm tests.   I'll try to describe things better 
>> below in reply to David's questions.
>>
>> On 11/11/2012 11:32 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> :)
>>>
>>> Can you explain the old and new relationships between these various 
>>> objects please. As I understand it JSR-292 introduced this special 
>>> kind of class - the anonymous class - and they differ from regular 
>>> classes in important ways (mainly that they can be collected before 
>>> their classloader), and pre-NPG this all "just worked". The NPG 
>>> changes added the ClassLoaderData metadata objects and changed some 
>>> of the associations that anonymous classes actually relied upon and 
>>> as a result they stopped getting GC'd in some cases, and were 
>>> prematurely GC'd in others (I hope I got that right). This changeset 
>>> addresses these problems.
>>
>> This is correct.  I don't think I could have written this better. 
>> Yes, in the old Permgen world, when the MethodHandles that contained 
>> the mirror to the anonymous class was unreferenced, the klass and all 
>> the metadata associated with it would be collected.   Now with 
>> ClassLoaderData objects, collection of metadata is tied to these 
>> objects.  The initial naive implementation just added the anonymous 
>> classes to the host_klass's ClassLoaderData object, which before b63 
>> (I think) was the boot class loader.  So the anonymous classes were 
>> never collected.
>>
>> This changeset gives each anonymous class it's own ClassLoaderData 
>> object and uses it's java_mirror to determine whether the anonymous 
>> class is still live and whether the ClassLoaderData object can be 
>> unloaded.
>>
>> We have been doing some tuning to determine how large the metaspace 
>> should be for these special cases.   That is ongoing.
>>
>>>
>>> I've been trying to follow this from the beginning and still don't 
>>> have a clear understanding on what the relationships between 
>>> ClassLoader, Class, "anonymous class" and ClassLoaderData objects 
>>> are. So a clear picture of what relationships exist (particularly 
>>> this new 1:N association) would be appreciated, and help make the 
>>> code changes more understandable to me.
>>
>> I can't draw pictures without a whiteboard!  But you may have several 
>> CLD objects that correspond to one class_loader oop. Each CLD object 
>> points to the class_loader, but the class_loader only points to the 
>> primary CLD object.   The additional CLD objects are for anonymous 
>> classes, which are kept live by their mirrors, not the class_loader.  
>> That's the source of many of the changes in this changeset.
>>
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 12/11/2012 1:51 PM, Coleen Phillimore wrote:
>>>>
>>>> This change creates a ClassLoaderData object for each JSR 292 
>>>> anonymous
>>>> class, so that the metadata and mirror object for the anonymous class
>>>> can be reclaimed when the anonymous class is no longer referenced. The
>>>> java_lang_ClassLoader object for the anonymous class is the same as 
>>>> its
>>>> host_class. Most of this change is to break the assumption that 
>>>> there's
>>>> a 1-1 relationship between java_lang_ClassLoader Java objects and
>>>> ClassLoaderData metadata objects in the VM. Also, nmethods and other
>>>> things that were strong roots for java_lang_ClassLoader objects 
>>>> have to
>>>> also be strong roots for java_lang_Class objects for anonymous 
>>>> classes.
>>>>
>>>> There were also changes to move the dependencies out of the
>>>> java_lang_ClassLoader object to the ClassLoaderData type. This type is
>>>> preallocated so that CMS will have card marks to track additions to 
>>>> the
>>>> dependencies. Please check this, Stefan!
>>>>
>>>> Also, in this change is the addition of mirrors to the interpreter 
>>>> frame
>>>> and a test case that shows why this is necessary. An interpreter frame
>>>> can be running while it's class loader is unloaded in some special
>>>> circumstances. It is easier to do this with JSR292 static MethodHandle
>>>> code. Some people are looking for a platform independent way to do 
>>>> this,
>>>> by changing code in GC. While this target-dependent interpreter 
>>>> code is
>>>> unfortunate, the concept is simple. If the latter effort succeeds, we
>>>> can remove the mirror from the interpreter frame later. A note to
>>>> openjdk developers - I added this mirror to zero but not to shark. 
>>>> More
>>>> testing is necessary.
>>>>
>>>> Please review the following change:
>>>>
>>>> Summary: Add ClassLoaderData object for each anonymous class with
>>>> metaspaces to allocate in. Add mirror interpreter frame.
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/8000662/
>>>> http://bugs.sun.com/view_bug.do?bug_id=8000662
>>>>
>>>> Tested with Nashorn tests, NSK full testlist, dacapo with CMS and G1.
>>>>
>>>> Thanks,
>>>> Coleen
>>
>