RFR: 8254315: Shenandoah: Concurrent weak reference processing [v12]

Tue Oct 27 11:30:29 UTC 2020

On Thu, 22 Oct 2020 16:04:25 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> Until now, references (as in java.lang.ref.Reference and its subclasses WeakReference, SoftReference, PhantomReference and the non-public FinalReference - I'll collectively call them weak references for the purpose of clarity). Workloads that make heavvy use of such weak references will therefore potentially cause significant GC pauses.
>> 
>> There are 3 main items that contribute to pause time linear to number of references, or worse:
>> - We need to scan and consider each reference on the various 'discovered' lists.
>> - We need to mark through subgraph of objects that are reachable only through FinalReference. Notice that this is theoretically only bounded by the live data set size.
>> - Finally, all no-longer-reachable references need to be enqueued in the 'pending list'
>> 
>> The problem is somewhat mitigated by pre-cleaning the discovered list: Any weak reference that we find to be strongly reachable will be removed before we go into the final-mark-pause. However, that is only a band-aid.
>> 
>> The solution to this is two-fold:
>> 1. Extend concurrent marking to also mark the 'finalizable' subgraph of the heap. This requires to extend the marking bitmap to allow for two kinds of reachability: each object can now be strongly and finalizably reachable. Whenever marking encounters a FinalReference, it will mark through the referent and switch to 'finalizably' reachability for all objects starting from the referent. When marking encounters finalizably reachable objects while marking strongly, it will 'upgrade' reachability of such objects to strongly reachable. All of this can be done concurrently. Any encounter of a Reference (or subclass) object will enqueue that object into a thread-local 'discovered' list. Except for FinalReference, marking stops there, and does not mark through the referent.
>> 2. Concurrent processing is performed after the final-mark pause. GC workers scan all discovered lists that have been collected by concurrent marking, and depending on reachability of the referent, either drop the Reference, or enqueue it into the global 'pending' list (from where it will be processed by Java reference handler thread). In addition to that, we must ensure that no referents become resurrected by accessing Reference.get() on it. In order to achieve this, we employ special barriers in Reference.get() intrinsics that return NULL when the referent is not reachable.
>> 
>> Testing: hotspot_gc_shenadoah (release+fastdebug, x86+aarch64), specjvm+specjbb without regressions, tier1, tier2, vmTestbase_vm_metaspace, vmTestbase_nsk_jvmti, with -XX:+UseShenandoahGC without regressions, specjvm with various levels of verification
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Rename native argument to maybe_narrow_oop for more clarity

My initial review follows. I have not digested the whole thing yet.

src/hotspot/cpu/aarch64/gc/shenandoah/shenandoahBarrierSetAssembler_aarch64.cpp line 360:

> 358: 
> 359:     if (ShenandoahBarrierSet::use_load_reference_barrier_native(decorators, type)) {
> 360:       load_reference_barrier_native(masm, dst, src, (decorators & IN_NATIVE) == 0);

I am a bit confused. If we introduce the local variable, would it be `maybe_narrow_oop`? How does it relate to `IS_NATIVE`? Also, see that `use_load_reference_barrier_native` already tests `IS_NATIVE`.

src/hotspot/cpu/x86/gc/shenandoah/shenandoahBarrierSetAssembler_x86.cpp line 525:

> 523: 
> 524:     if (ShenandoahBarrierSet::use_load_reference_barrier_native(decorators, type)) {
> 525:       load_reference_barrier_native(masm, dst, src, (decorators & IN_NATIVE) == 0);

Same comment as in `aarch64` code.

src/hotspot/share/gc/shenandoah/c1/shenandoahBarrierSetC1.cpp line 287:

> 285:     _load_reference_barrier_weakref_rt_code_blob = Runtime1::generate_blob(buffer_blob, -1,
> 286:                                                                           "shenandoah_load_reference_barrier_weakref_slow",
> 287:                                                                           false, &lrb_weakref_code_gen_cl);

Nit: Looks like indenting is a bit off here, in comparisons with blocks above.

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.hpp line 41:

> 39:     NATIVE,
> 40:     WEAK
> 41:   };

Descriptions maybe? Also, `ShenandoahLRBKind` seems noisy. Since it is already in `SBS`, maybe `ShenandoahBarrierSet::LRBKind`?

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.hpp line 64:

> 62:   static bool use_load_reference_barrier_native(DecoratorSet decorators, BasicType type);
> 63:   static bool need_keep_alive_barrier(DecoratorSet decorators, BasicType type);
> 64:   static ShenandoahLRBKind access_kind(DecoratorSet decorators, BasicType type);

...or in fact, `ShenandoahBarrierSet::AccessKind` to match the `access_kind` here? Or does it clash with something else? Or rename this to `lrb_kind`?

src/hotspot/share/gc/shenandoah/c2/shenandoahBarrierSetC2.cpp line 1062:

> 1060:     Node* in2 = n->in(2);
> 1061:     if (in1->bottom_type() == TypePtr::NULL_PTR &&
> 1062:         (in2->Opcode() != Op_ShenandoahLoadReferenceBarrier ||

This is a bugfix, right? It changes `in1` (seemingly incorrect) to `in2` (seemingly correct). If so, maybe we should split it out to fix previous releases too?

src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp line 996:

> 994:       break;
> 995:     default:
> 996:       ShouldNotReachHere();

I expect some compilers to complain here about the uninitialized `name`, please add `name = NULL;` before the `ShouldNotReachHere()`?

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.hpp line 104:

> 102: 
> 103:   template<typename T>
> 104:   inline oop load_reference_barrier_native(oop obj, T* load_addr);

These might be forked to a separate cleanup? Not insisting... That would make backports a bit cleaner, though.

src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp line 81:

> 79:   _heap(ShenandoahHeap::heap()),
> 80:   _mark_context(_heap->marking_context()),
> 81:   _strong(true)

Do we want to turn this to yet another template parameter, like for dedup? That would also resolve passing `true` or `false` to `strong` argument without comments.

src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.inline.hpp line 264:

> 262:         marked = mark_context->mark_strong(obj, marked_first);
> 263:       } else {
> 264:         marked = mark_context->mark_final(obj, marked_first);

Is this `mark_final` actually `mark_weak`?

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp line 64:

> 62: }
> 63: 
> 64: inline bool ShenandoahMarkBitMap::mark_final(HeapWord* heap_addr, bool& marked_first) {

It looks to me that `marked_first` is always the same as the return value? If so, can we drop that out-argument?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1733:

> 1731:       }
> 1732:     }
> 1733: 

Why this move?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 3053:

> 3051: 
> 3052:   ShenandoahWorkerScope scope(workers(),
> 3053:                               ShenandoahWorkerPolicy::calc_workers_for_conc_root_processing(),

It probably does not matter, but maybe we should be having a separate `ShenandoahWorkerPolicy` entry for `conc_weak_refs`.

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 404:

> 402:   assert(ctx->is_complete(), "sanity");
> 403: 
> 404:   const ShenandoahMarkBitMap* mark_bit_map = ctx->mark_bit_map();

Why `const`? Not necessarily wrong, but inconsistent with the rest of the method.

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2020, Red Hat, Inc. and/or its affiliates.

Was it copied from the `MarkBitMap`? Oracle's copyright needs to be left in place, methinks, along with Red Hat-s for Red Hat-s modifications.

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2020, Red Hat, Inc. and/or its affiliates.

Ditto.

src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.inline.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2020, Red Hat, Inc. and/or its affiliates.

Ditto.

src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.hpp line 85:

> 83:   f(conc_weak_refs,                                 "Concurrent Weak References")      \
> 84:   f(conc_weak_refs_work,                            "  Process")                       \
> 85:   SHENANDOAH_PAR_PHASE_DO(conc_weak_refs_work_,     "    CWR: ", f)                    \

Eh. Clashes with `CWR:` below... Maybe `CWRF`?

src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.hpp line 42:

> 40:  * 1. Concurrent reference marking: Discover all j.l.r.Reference objects and determine reachability of all live objects.
> 41:  * 2. Concurrent reference processing: For all discoved j.l.r.References, determine whether or not to keep or clean
> 42:  *    them. Also, clean and enqueue relevant references concurrently.

"determine whether to keep them alive or clean them", right? These are the choices?

src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.hpp line 53:

> 51:  * These reachabilities are implemented in shenandoahMarkBitMap.*
> 52:  * Conceptually, marking starts with a strong wavefront at the GC roots. Whenever a Reference object is encountered,
> 53:  * that Reference is discovered, it may be discovered by the ShenandoahReferenceProcessor. If it is discovered, it

"Whenever a Reference object is encountered, it may be discovered by the ShenandoahReferenceProcessor"?

src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.hpp line 55:

> 53:  * that Reference is discovered, it may be discovered by the ShenandoahReferenceProcessor. If it is discovered, it
> 54:  * gets added to the discovered list, and that wavefront stops there, except when it's a FinalReference, in which
> 55:  * case the wavefront switches to finalizable marking and marks through the refenent. When a Reference is not

"refenent" -> "referent"

src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.hpp line 57:

> 55:  * case the wavefront switches to finalizable marking and marks through the refenent. When a Reference is not
> 56:  * discovered, e.g. if it's a SoftReference that is not eligible for discovery, then marking continues as if the
> 57:  * Reference were a regular object. Whenever a strong wavefront encounters an object that is already marked

"was a regular object"

src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.hpp line 154:

> 152: public:
> 153:   ObjArrayChunkedTask(oop o = NULL, bool count_liveness = true, bool strong = true) {
> 154:     assert(decode_oop(encode_oop(o, count_liveness, strong)) ==  o, "oop can be encoded: " PTR_FORMAT, p2i(o));

Need encodeability `assert`-s for `count_liveness` and `strong` too? Also in the other constructor? This might get costly, and the whole thing might need rethinking... But at least the initial testing better check there are no bugs here.

src/hotspot/share/gc/shenandoah/shenandoahTaskqueue.hpp line 180:

> 178:   inline uintptr_t encode_oop(oop obj, bool count_liveness, bool strong) const {
> 179:     uintptr_t encoded_oop = ((uintptr_t)(void*) obj) << oop_shift;
> 180:     assert((encoded_oop & (count_liveness_decode_mask | strong_decode_mask)) == 0, "need bit for encoding count-liveness and strong bits");

Ah! This assert can be sacrificed for the additional asserts in constructors, right?

src/hotspot/share/gc/shenandoah/shenandoahThreadLocalData.hpp line 55:

> 53:   int  _disarmed_value;
> 54:   double _paced_time;
> 55:   ShenandoahMarkRefsSuperClosure* _mark_closure;

This rubs me the wrong way. Closures are usually stack-allocated, so we are exposing the stack pointer here.

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 97:

> 95:     if (!CompressedOops::is_null(o)) {
> 96:       oop obj = CompressedOops::decode_not_null(o);
> 97:       obj = ShenandoahForwarding::get_forwardee(obj);

But... `verify_oop` verifies the consistency of `obj` and its fwdptr. Here, we effectively omit those checks, making verification less effective! Is this for `Reference` classes only?

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 853:

> 851:           _verify_forwarded_none,                  // no forwarded references
> 852:           _verify_marked_complete_except_references, // walk over marked objects too
> 853:           _verify_cset_disable,                        // non-forwarded references to cset expected

Please make sure these are indented properly.

-------------

Changes requested by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/505