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

[Christian Thalinger]

> On Apr 19, 2016, at 4:29 PM, Coleen Phillimore <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> 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
>>> 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