Review request (M): 8003720: NPG: Method in interpreter stack frame can be deallocated

[John Coomes]

Stefan Karlsson (stefan.karlsson at oracle.com) wrote:
> David,
> 
> Thanks for taking a look at this.
> 
> I've added a comment regarding the cld_f:
> 
>      // GC support
>      // Apply "f->do_oop" to all root oops in "this".
> *+   // Apply "cld_f->do_cld" to CLDs that are otherwise not kept alive.*
> *+   //   Used by JavaThread::oops_do.*
>      // Apply "cf->do_code_blob" (if !NULL) to all code blobs active in frames
> *!    virtual void oops_do(OopClosure* f,_CLDToOopClosure* cld_f,_CodeBlobClosure* cf);*
> 
> http://cr.openjdk.java.net/~stefank/8003720/webrev.01

iterator.hpp:
------------

I don't see any uses of this constructor:

 150   CLDToOopClosure(OopClosure* oop_closure, KlassClosure* klass_closure, bool must_claim_cld = true) :
 151       _oop_closure(oop_closure),
 152       _default_klass_closure(NULL), // Ignored
 153       _klass_closure(klass_closure),
 154       _must_claim_cld(must_claim_cld) {}

If it's unused, then you can eliminate the _default_klass_closure /
_klass_closure distinction.

frame.cpp:
--------

A typo in the comment:

916     // closure that knows have to keep klasses alive given a ClassLoaderData.
                              ^^^^ 

Otherwise, looks good.

-John

> On 11/23/2012 01:01 AM, David Holmes wrote:
> > Hi Stefan,
> >
> > Not my area so a minor comment not a review :)
> >
> > thread.hpp
> >
> >  481   // GC support
> >  482   // Apply "f->do_oop" to all root oops in "this".
> >  483   // Apply "cf->do_code_blob" (if !NULL) to all code blobs active 
> > in frames
> >  484   virtual void oops_do(OopClosure* f, CLDToOopClosure* cld_f, 
> > CodeBlobClosure* cf);
> >
> > can you add a comment that cld_f is not used/needed by Thread::oops_do
> >
> > Thanks,
> > David
> >
> > On 23/11/2012 12:14 AM, Stefan Karlsson wrote:
> >> http://cr.openjdk.java.net/~stefank/8003720/webrev/
> >>
> >> Description from CR:
> >> In virtual calls the Method pointer in the interpreter stack frame is
> >> not kept alive by anything other than the "this" pointers to that
> >> method. If bytecodes overwrite the "this" pointer, then call a full GC,
> >> the class loader containing the Method* can be unloaded and the Method*
> >> deallocated.
> >>
> >> This is also a problem with JSR292 MethodHandle static code because the
> >> MethodHandle containing the mirror for the interpreted method Method* is
> >> not on the stack if a GC occurs.
> >>
> >> Fix proposal:
> >> The "obvious" solution to this problem would be to apply the root
> >> scanning OopClosure to the Klass::_java_mirror field of the method in
> >> the interpreted frame. However, doing this might cause us to scan the
> >> same metadata oop location more than once, which is not allowed by some
> >> of the HotSpot GCs. We currently solve similar situations by always
> >> "claiming" and start scanning from the ClassLoaderData and then proceed
> >> down into the Klasses of that class loader.
> >>
> >> For this bug we do the same. All old collections, where class unloading
> >> can occur, pass down a closure that is applied to the ClassLoaderData of
> >> the Klass of the Method in the interpreted frame. This closure does the
> >> claiming and proceeds to scan the class metadata. Note that during young
> >> collections, where we don't do class unloading, all classes are already
> >> used as strong roots and we don't have to apply this new closure in the
> >> interpreted frame.
> >>
> >> Testing:
> >> The added test was initially written by John Rose. I only ported it to
> >> JTreg and made some artistic cleanups to it.
> >>
> >> thanks,
> >> StefanK
>