RFR (11u, XXL): Upstream/backport Shenandoah to JDK11u - the review thread
Roman Kennke
rkennke at redhat.com
Fri Jul 24 11:50:55 UTC 2020
Hi Aditya,
thank you for reviewing! See my comments in-inline:
> Hi Roman,
>
> I took a first pass at reviewing the shared code changes that are not
> under test/, with an eye toward
> ways in which the patch could affect Shenandoah-less builds. Things
> mostly looked good, and many of these
> are nitpicks.
>
> The review below is based on
> http://cr.openjdk.java.net/~rkennke/shenandoah-jdk11u-upstream/webrev.10-shared/
>
> As a disclaimer, I'm not currently an official reviewer on the
> project.
>
> Thanks,
> Aditya
>
> Here it goes:
>
> ============================================================
> src/hotspot/share/opto/escape.cpp, ~590:
>
> @@ -554,8 +567,8 @@
> // Pointer stores in G1 barriers looks like unsafe access.
> // Ignore such stores to be able scalar replace non-
> escaping
> // allocations.
> -#if INCLUDE_G1GC
> - if (UseG1GC && adr->is_AddP()) {
> +#if INCLUDE_G1GC || INCLUDE_SHENANDOAHGC
> + if ((UseG1GC SHENANDOAHGC_ONLY(|| UseShenandoahGC)) &&
> adr->is_AddP()) {
> Node* base = get_addp_base(adr);
> if (base->Opcode() == Op_LoadP &&
> base->in(MemNode::Address)->is_AddP()) {
>
> @@ -563,7 +576,15 @@
> Node* tls = get_addp_base(adr);
> if (tls->Opcode() == Op_ThreadLocal) {
> int offs = (int)igvn->find_intptr_t_con(adr-
> >in(AddPNode::Offset), Type::OffsetBot);
> - if (offs ==
> in_bytes(G1ThreadLocalData::satb_mark_queue_buffer_offset())) {
> +#if INCLUDE_G1GC && INCLUDE_SHENANDOAHGC
> + const int buf_offset = in_bytes(UseG1GC ?
> G1ThreadLocalData::satb_mark_queue_buffer_offset()
> + :
> ShenandoahThreadLocalData::satb_mark_queue_buffer_offset());
> +#elif INCLUDE_G1GC
> + const int buf_offset =
> in_bytes(G1ThreadLocalData::satb_mark_queue_buffer_offset());
> +#else
> + const int buf_offset =
> in_bytes(ShenandoahThreadLocalData::satb_mark_queue_buffer_offset());
> +#endif
> + if (offs == buf_offset) {
> break; // G1 pre barrier previous oop value store.
> }
> if (offs ==
> in_bytes(G1ThreadLocalData::dirty_card_queue_buffer_offset())) {
>
> -----
>
> The outermost block has been relaxed to be under `#if
> INCLUDE_G1GC || INCLUDE_SHENANDOAHGC`, but I think
> that last line above (the call to
> G1ThreadLocalData::dirty_card_queue_buffer_offset()) won't compile if
> G1
> is disabled, will it? I haven't verified it myself, but you may
> want to check that. I assume including
> Shenandoah and not G1 is a valid configuration based on the other
> checks you added.
>
I think you're missing the final #endif after the block that you showed
here. The one that closes the #if INCLUDE_G1GC || INCLUDE_SHENANDOAHGC.
I think it's ok. Also notice that
G1ThreadLocalData::dirty_card_queue_buffer_offset() will never happen
with Shenandoah, we don't use the dirty_card_queue.
> ============================================================
> src/hotspot/cpu/x86/c1_LIRGenerator_x86.cpp, ~681:
>
> #ifdef ASSERT
> @@ -674,6 +675,11 @@
> if (type == T_OBJECT || type == T_ARRAY) {
> cmp_value.load_item_force(FrameMap::rax_oop_opr);
> new_value.load_item();
> +#if INCLUDE_SHENANDOAHGC
> + if (UseShenandoahGC) {
> + __ cas_obj(addr->as_address_ptr()->base(), cmp_value.result(),
> new_value.result(), new_register(T_OBJECT), new_register(T_OBJECT));
> + } else
> +#endif
> __ cas_obj(addr->as_address_ptr()->base(), cmp_value.result(),
> new_value.result(), ill, ill);
> } else if (type == T_INT) {
>
>
> -----
>
> The hanging brace-less else scares me a bit since someone might
> add something else after the
> endif and unknowingly cause problems for Shenandoah builds.
>
Yes. We've actually had what you propose below. For some reason it
compiles slightly different code, and my comparison script would
complain about it. The alternative would be to put in extra #if for the
closing }, but that looks even messier.
> ============================================================
> src/hotspot/cpu/x86/c1_LIRGenerator_x86.cpp, ~708:
>
> @@ -699,6 +705,12 @@
> // Because we want a 2-arg form of xchg and xadd
> __ move(value.result(), result);
> assert(type == T_INT || is_oop LP64_ONLY( || type == T_LONG ),
> "unexpected type");
> +#if INCLUDE_SHENANDOAHGC
> + if (UseShenandoahGC) {
> + LIR_Opr tmp = is_oop ? new_register(type) :
> LIR_OprFact::illegalOpr;
> + __ xchg(addr, result, result, tmp);
> + } else
> +#endif
> __ xchg(addr, result, result, LIR_OprFact::illegalOpr);
> return result;
> }
>
> -----
>
> This is another case like the above (and in the same file).
>
> I understand there's a motovation to keep the non-Shenandoah code
> delta minimal, but something like this seems much nicer:
>
>
> LIR_Opr tmp = LIR_OprFact::illegalOpr;
>
> #if INCLUDE_SHENANDOAHGC
> if (UseShenandoahGC && is_oop) {
> tmp = new_register(type);
> #endif
>
> __ xchg(addr, result, result, tmp);
> return result;
>
Yes, but see above.
> ============================================================
> src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp, two places
>
> @@ -2234,6 +2245,14 @@
> switch (in_sig_bt[i]) {
> case T_ARRAY:
> if (is_critical_native) {
> +#if INCLUDE_SHENANDOAHGC
> + // pin before unpack
> + if (UseShenandoahGC) {
> + assert(pinned_slot <= stack_slots, "overflow");
> + ShenandoahBarrierSet::assembler()-
> >pin_critical_native_array(masm, in_regs[i], pinned_slot);
> + pinned_args.append(i);
> + }
> +#endif
>
> @@ -2450,6 +2469,22 @@
> default : ShouldNotReachHere();
> }
>
> +#if INCLUDE_SHENANDOAHGC
> + if (UseShenandoahGC) {
> + // unpin pinned arguments
> + pinned_slot = oop_handle_offset;
> + if (pinned_args.length() > 0) {
> + // save return value that may be overwritten otherwise.
> + save_native_result(masm, ret_type, stack_slots);
> + for (int index = 0; index < pinned_args.length(); index ++) {
> + int i = pinned_args.at(index);
> + assert(pinned_slot <= stack_slots, "overflow");
> + ShenandoahBarrierSet::assembler()-
> >unpin_critical_native_array(masm, in_regs[i], pinned_slot);
> + }
> + restore_native_result(masm, ret_type, stack_slots);
> + }
> + }
> +#endif
>
> -----
>
> Minor issue: There are tab characters used in indenting the
> ShenandoahBarrierSet* lines, inconsistent
> with the surrounding code.
Good spot! Fixed in webrev.11 (see below).
> ============================================================
> Various files
>
> - * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All
> rights reserved.
> + * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All
> rights reserved.
>
> -----
>
> What's the normal policy around how these years are updated? If
> the change is being made now, should
> they be 2020?
Those copyright updates stem from when we actually made those changes
in shenandoah/jdk11 repository. But we should decide what to do with
this? Leave it like I proposed (i.e. according to original changes from
shenandoah/jdk11), or remove all copyright changes, or update all
touched files to 2020?
Also, we should agree on the commit message if this gets approved. I
currently set it to just 'Shenandoah: A Low-Pause-Time Garbage
Collector'. Does it require a bug-ID? If yes, then which one? A
backport-issue of what? The original Shenandoah JEP 189 implementation?
A backport of the new JEP 379, which seems more fitting in the title,
and also represents the current state of Shenandoah overall better, but
a wholly different patch in jdk11u?
> ============================================================
> src/hotspot/share/gc/shared/gcConfig.cpp, ~60
>
> + CMSGC_ONLY(static CMSArguments cmsArguments;)
> + EPSILONGC_ONLY(static EpsilonArguments epsilonArguments;)
> + G1GC_ONLY(static G1Arguments g1Arguments;)
> + PARALLELGC_ONLY(static ParallelArguments parallelArguments;)
> + SERIALGC_ONLY(static SerialArguments serialArguments;)
> +SHENANDOAHGC_ONLY(static ShenandoahArguments shenandoahArguments;)
> + ZGC_ONLY(static ZArguments zArguments;)
>
> -----
>
> Minor issue: alignment of the others needs to be adjusted
Right. Fixed in webrev.11.
> ============================================================
> src/hotspot/share/gc/shared/gcConfig.cpp, ~98
>
> @@ -90,14 +95,14 @@
> bool GCConfig::_gc_selected_ergonomically = false;
>
> void GCConfig::fail_if_unsupported_gc_is_selected() {
> - NOT_CMSGC( FAIL_IF_SELECTED(UseConcMarkSweepGC, true));
> - NOT_EPSILONGC( FAIL_IF_SELECTED(UseEpsilonGC, true));
> - NOT_G1GC( FAIL_IF_SELECTED(UseG1GC, true));
> - NOT_PARALLELGC(FAIL_IF_SELECTED(UseParallelGC, true));
> - NOT_PARALLELGC(FAIL_IF_SELECTED(UseParallelOldGC, true));
> - NOT_SERIALGC( FAIL_IF_SELECTED(UseSerialGC, true));
> - NOT_SERIALGC( FAIL_IF_SELECTED(UseParallelOldGC, false));
> - NOT_ZGC( FAIL_IF_SELECTED(UseZGC, true));
> + NOT_CMSGC( FAIL_IF_SELECTED(UseConcMarkSweepGC, true));
> + NOT_EPSILONGC( FAIL_IF_SELECTED(UseEpsilonGC, true));
> + NOT_G1GC( FAIL_IF_SELECTED(UseG1GC, true));
> + NOT_PARALLELGC( FAIL_IF_SELECTED(UseParallelGC, true));
> + NOT_PARALLELGC( FAIL_IF_SELECTED(UseParallelOldGC, true));
> + NOT_SERIALGC( FAIL_IF_SELECTED(UseSerialGC, true));
> + NOT_SERIALGC( FAIL_IF_SELECTED(UseParallelOldGC, false));
> + NOT_ZGC( FAIL_IF_SELECTED(UseZGC, true));
>
> -----
>
> Minor issue: These whitespace changes can be undone. (I guess a
> prior version had a Shenandoah thing
> added here which has now been removed?)
Right. Fixed in webrev.11.
> ============================================================
> src/hotspot/share/runtime/stackValue.cpp, ~114
>
> - // Decode narrowoop and wrap a handle around the oop
> - Handle h(Thread::current(),
> CompressedOops::decode(value.noop));
> + // Decode narrowoop
> + oop val = CompressedOops::decode(value.noop);
> + // Deoptimization must make sure all oops have passed load
> barriers
> +#if INCLUDE_SHENANDOAHGC
> + if (UseShenandoahGC) {
> + val = ShenandoahBarrierSet::barrier_set()-
> >load_reference_barrier(val);
> + }
> +#endif
> + Handle h(Thread::current(), val); // Wrap a handle around the
> oop
>
> -----
>
> I'd move the load barrier comment into the `if (UseShenandoahGC)`
> for better readability.
>
Right. Fixed in webrev.11.
> ============================================================
> src/hotspot/share/gc/shared/vmStructs_gc.hpp, three places
>
> SERIALGC_ONLY(VM_STRUCTS_SERIALGC(nonstatic_field,
> \
> volatile_nonstatic_field,
> \
> static_field))
> \
> + SHENANDOAHGC_ONLY(VM_STRUCTS_SHENANDOAH(nonstatic_field,
> \
> + volatile_nonstatic_field,
> \
> + static_field))
>
> -----
>
> Ultra nit-pick: The others have the second and third arguments
> aligned with the first. There
> are three places in this file where you've added the Shenandoah
> ones and they're not aligned
> this way. Hey, I warned you some of these comments are minor :).
Right. The last one doesn't quite fit, and I'm not going to change
*all* of the trailing \s. Fixed in webrev.11.
Full:
Shared-only:
Testing: re-did my object-file comparison. Still clean. Built with and
without Shenandoah enabled, run hotspot_gc_shenandoah fine.
Thank you,
Roman
More information about the jdk-updates-dev
mailing list