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

John Coomes John.Coomes at oracle.com
Mon Nov 26 16:10:57 PST 2012

Stefan Karlsson (stefan.karlsson at oracle.com) wrote:

David,

Thanks for taking a look at this. I've added a comment regarding the cldf: // GC support // Apply "f->dooop" to all root oops in "this". + // Apply "cldf->docld" to CLDs that are otherwise not kept alive. + // Used by JavaThread::oopsdo. // Apply "cf->docodeblob" (if !NULL) to all code blobs active in frames ! virtual void oopsdo(OopClosure f,CLDToOopClosure* cldf,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->dooop" to all root oops in "this". > 483 // Apply "cf->docodeblob" (if !NULL) to all code blobs active > in frames > 484 virtual void oopsdo(OopClosure* f, CLDToOopClosure* cldf, > CodeBlobClosure* cf); > > can you add a comment that cldf is not used/needed by Thread::oopsdo > > 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::javamirror 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

More information about the hotspot-dev mailing list