RFR (L) 8154580: Save mirror in interpreter frame to enable cleanups of CLDClosure

[Coleen Phillimore]

Thanks Chris!
Coleen

On 4/20/16 5:43 PM, Christian Thalinger wrote:
>
>> On Apr 19, 2016, at 4:29 PM, Coleen Phillimore 
>> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>> 
>> wrote:
>>
>>
>> Chris, thank you for reviewing this.
>>
>> On 4/19/16 4:35 PM, Christian Thalinger wrote:
>>>> On Apr 19, 2016, at 9:50 AM, Coleen Phillimore 
>>>> <coleen.phillimore at oracle.com 
>>>> <mailto:coleen.phillimore at oracle.com>> wrote:
>>>>
>>>> Summary: GC walks the mirror using OopClosure rather than using 
>>>> CLDClosure in oops_interpreted_do()
>>>>
>>>> See bug for more description and justification.  The changes are 
>>>> large but very redundant.  The main change is in 
>>>> TemplateInterpreterGenerator::generate_fixed_frame().
>>> +  // Save oop Mirror (with padding)
>>> +  __ load_mirror(rscratch1, rmethod);
>>>
>>> +  // get mirror and store it in the frame so that this Method* is never
>>> +  // reclaimed while it's running.
>>> +  Register mirror = LcpoolCache;
>>> +  __ load_mirror(mirror, Method);
>>>
>>> +  // Push the mirror so this method isn't collected
>>> +  __ load_mirror(rdx, rbi);
>>>
>>> Please use the same comment on all platforms.
>>
>> Yes, that's inconsistent.  I changed all platforms to have this comment:
>>
>>  // Get mirror and store it in the frame as GC root for this Method*
>>
>>>> open webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/8154580.01/webrev 
>>>> <http://cr.openjdk.java.net/%7Ecoleenp/8154580.01/webrev>
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8154580
>>> src/share/vm/runtime/frame.cpp
>>>
>>>      // The method pointer in the frame might be the only path to 
>>> the method's
>>>      // klass, and the klass needs to be kept alive while executing. 
>>> The GCs
>>>      // don't trace through method pointers, so typically in similar 
>>> situations
>>>      // the mirror or the class loader of the klass are installed as 
>>> a GC root.
>>>
>>> -    // To minimize the overhead of doing that here, we ask the GC 
>>> to pass down a
>>> -    // closure that knows how to keep klasses alive given a 
>>> ClassLoaderData.
>>> -    cld_f->do_cld(m->method_holder()->class_loader_data());
>>> -  }
>>> -
>>> -  if (m->is_native() PPC32_ONLY(&& m->is_static())) {
>>> -    f->do_oop(interpreter_frame_temp_oop_addr());
>>> -  }
>>> +  // And it is here too.
>>> +  f->do_oop(interpreter_frame_mirror_addr());
>>>
>>> That comment is kinda funny now.  It still hints at the old-way of 
>>> doing things but “it is here too”.
>>
>> I reworded it as:
>>
>>  // The method pointer in the frame might be the only path to the 
>> method's
>>  // klass, and the klass needs to be kept alive while executing. The GCs
>>  // don't trace through method pointers, so the mirror of the 
>> method's klass
>>  // is installed as a GC root.
>>  f->do_oop(interpreter_frame_mirror_addr());
>
> Excellent.
>
>>
>>>> Tested with hotspot-runtime-nightly and gc-nightly tests.
>>>>
>>>> Need testing with ppc and aarch64 open code.  I implemented the 
>>>> changes but I can't test them.
>>> One obvious bug is that you copied the __ as well:
>>>
>>> +void MacroAssembler::load_mirror(Register dst, Register method) {
>>> +  const int mirror_offset = in_bytes(Klass::java_mirror_offset());
>>> +  __ ldr(dst, Address(rmethod, Method::const_offset()));
>>> +  __ ldr(dst, Address(dst, ConstMethod::constants_offset()));
>>> +  __ ldr(dst, Address(dst, 
>>> ConstantPool::pool_holder_offset_in_bytes()));
>>> +  __ ldr(dst, Address(dst, mirror_offset));
>>> +}
>>>
>>> Other than that it looks fine to me.  Nothing obvious stood out.
>>
>> Oh, yes, this was wrong on both ppc and aarch64.   I hope someone 
>> that has access to this platforms can test this patch.
>>
>> Thank you!
>> Coleen
>>
>>>
>>>> Thanks,
>>>> Coleen
>