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

[Stefan Karlsson]

On 11/27/2012 01:10 AM, John Coomes wrote:
> 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.

Good catch. I'll remove the distinction.

>
> frame.cpp:
> --------
>
> A typo in the comment:
>
> 916     // closure that knows have to keep klasses alive given a ClassLoaderData.
>                                ^^^^

Fixed.

>
> Otherwise, looks good.

Thanks for the review.

StefanK

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