RFR (L) 8154580: Save mirror in interpreter frame to enable cleanups of CLDClosure
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Apr 20 02:29:08 UTC 2016
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());
>> 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
More information about the hotspot-dev
mailing list