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

[Stefan Karlsson]

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.


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.


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) {

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");


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;
    }


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);
      }

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
>