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

Stefan Karlsson stefan.karlsson at oracle.com
Thu Nov 29 04:12:55 PST 2012

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:

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.

is_alive->do_object_b(data->class_loader())) {

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     {

class loader can never be unloaded");

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

InstanceKlass::cast(accessing_klass())->class_loader_data();

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/80006623/ bug link at http://bugs.sun.com/viewbug.do?bugid=80006623 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 hostklass'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 javamirror 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 classloader oop. Each CLD object points to the classloader, but the classloader only points to the primary CLD object. The additional CLD objects are for anonymous classes, which are kept live by their mirrors, not the classloader. 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 javalangClassLoader object for the anonymous class is the same as its hostclass. Most of this change is to break the assumption that there's a 1-1 relationship between javalangClassLoader Java objects and ClassLoaderData metadata objects in the VM. Also, nmethods and other things that were strong roots for javalangClassLoader objects have to also be strong roots for javalangClass objects for anonymous classes. There were also changes to move the dependencies out of the javalangClassLoader 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/viewbug.do?bugid=8000662 Tested with Nashorn tests, NSK full testlist, dacapo with CMS and G1. Thanks, Coleen

More information about the hotspot-dev mailing list