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

[Stefan Karlsson]

On 29 nov 2012, at 18:38, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:

> 
> 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 ...

Thanks for changing this.

I think it would be good to move the data->keep_alive() check into the is_alive function.

StefanK

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