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

[Christian Thalinger]

On Nov 11, 2012, at 7:51 PM, Coleen Phillimore <coleen.phillimore at oracle.com> 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.

We should not push this change and back it out later.  Let's pick the right one before pushing.

The simplest fix I have (after trying various other fixes) is putting the java mirror in a Handle when doing the Threads::gc_prologue stack walk:

diff --git a/src/share/vm/runtime/frame.cpp b/src/share/vm/runtime/frame.cpp
--- a/src/share/vm/runtime/frame.cpp
+++ b/src/share/vm/runtime/frame.cpp
@@ -1152,6 +1152,11 @@
   if (is_interpreted_frame()) {
     // set bcx to bci to become Method* position independent during GC
     interpreter_frame_set_bcx(interpreter_frame_bci());
+
+    // Keep the class alive that holds this Method*
+    Thread* thread = Thread::current();
+    methodHandle m(thread, interpreter_frame_method());
+    Handle h(m()->constants()->pool_holder()->java_mirror());
   }
 }

Threads::gc_prologue walks are only done during full GCs but we talked in length with the GC people and Stefan Karlsson on Friday and they agreed that the fix is correct.  I did a lot of 262 test suite runs with Nashorn and different GCs without a crash.

The only downside is that for CMS we need to add an additional call to this walking routine in CMSCollector::checkpointRootsFinalWork:

diff --git a/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp b/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
--- a/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
+++ b/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
@@ -4868,6 +4868,7 @@
 
   if (should_unload_classes()) {
     CodeCache::gc_prologue();
+    Threads::gc_prologue();
   }
   assert(haveFreelistLocks(), "must have free list locks");
   assert_lock_strong(bitMapLock());
@@ -4928,6 +4929,7 @@
 
   if (should_unload_classes()) {
     CodeCache::gc_epilogue();
+    Threads::gc_epilogue();
   }
   JvmtiExport::gc_epilogue();
 
This adds a small overhead but it seems to be possible to eliminate that overhead with some refactoring in CMS (IIRC).

There is also another fix from Stefan Karlsson which I'll post later.

-- Chris

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