RFR: 8256011: Shenandoah: Don't resurrect finalizably reachable objects [v3]

[Aleksey Shipilev]

On Tue, 10 Nov 2020 09:32:09 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> In the weak-LRB we currently return referents when it is 'marked', that is when it's either reachable strongly or through a finalizable object. This means a finalizable object can be resurrected by Reference.get(), which is wrong. Only truly strongly reachable objects should be returned by Reference.get() during weak-reference-processing. 
>> 
>> Testing: hotspot_gc_shenandoah, tier1 & tier2 with -XX:+UseShenandoahGC
>
> Roman Kennke has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - Ensure correct strength and width of LRB runtime calls
>  - Merge branch 'master' into JDK-8256011
>  - Merge branch 'master' into JDK-8256011
>  - Simplify condition, don't resurrect finalizably reachable objects even on phantom access
>  - 8256011: Shenandoah: Don't resurrect finalizably reachable objects

I have many questions :)

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

> 861:     __ call(RuntimeAddress(bs->load_reference_barrier_weak_rt_code_blob()->code_begin()));
> 862:   } else {
> 863:     assert(is_phantom, "only remaining strenght");

Typo: "strenght"

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

> 942:     if (UseCompressedOops) {
> 943:       __ call_VM_leaf(CAST_FROM_FN_PTR(address, ShenandoahRuntime::load_reference_barrier_strong_narrow), c_rarg0,
> 944:                       c_rarg1);

Stick with newline style? Is `c_rarg1` on new line or not in this method?

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

> 1061:     Node* in2 = n->in(2);
> 1062: 
> 1063:     // If one input is NULL, then step over the barriers normal LRB barriers on the other input

"strong LRBs barriers" now... :)

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

> 982:       calladdr = CAST_FROM_FN_PTR(address, ShenandoahRuntime::load_reference_barrier_strong);
> 983:     }
> 984:     name = "load_reference_barrier_strong";

Why not separate "load_reference_barrier_strong" and  "load_reference_barrier_strong_narrow"?

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

> 2908: 
> 2909: ShenandoahLoadReferenceBarrierNode::ShenandoahLoadReferenceBarrierNode(Node* ctrl, Node* obj, DecoratorSet decorators)
> 2910: : Node(ctrl, obj), _decorators(decorators & (ON_STRONG_OOP_REF | ON_WEAK_OOP_REF | ON_PHANTOM_OOP_REF | ON_UNKNOWN_OOP_REF | IN_NATIVE)) {

Do we really want to strip bits from decorator here? Aren't we testing specific bits anyway downstream? I can imagine some downstream code in the future would be surprised not to see some bits that are actually set, but stripped here?

Is this for `LRBNode::hash/cmp` stability? It should be stripped in `hash/cmp` then.

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

> 56: 
> 57:   static bool is_strong_access(DecoratorSet decorators) {
> 58:     return (decorators & (ON_WEAK_OOP_REF | ON_PHANTOM_OOP_REF | ON_UNKNOWN_OOP_REF)) == 0;

Is this the same as `decorators & ON_STRONG_OOP_REF`?

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 105:

> 103: inline oop ShenandoahBarrierSet::load_reference_barrier(oop obj, T* load_addr) {
> 104: 
> 105:   // Prevent resurrection of unreachable non-strorg references.

A chance to fix typo "non-strorg" here.

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

> 60: 
> 61:   static bool is_weak_access(DecoratorSet decorators) {
> 62:     return (decorators & (ON_WEAK_OOP_REF | ON_UNKNOWN_OOP_REF)) != 0;

Um. Shouldn't `ON_UNKNOWN_OOP_REF` default to strong? I.e. the access via Unsafe should probably resurrect the object, otherwise it corrupts the heap?

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 108:

> 106:   if (!HasDecorator<decorators, ON_STRONG_OOP_REF>::value && obj != NULL &&
> 107:       _heap->is_concurrent_weak_root_in_progress() &&
> 108:       !(HasDecorator<decorators, ON_PHANTOM_OOP_REF>::value ? _heap->marking_context()->is_marked(obj)

Maybe split out boolean locals for decorator tests? A matter of style, your call.

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

> 1064:     if (in1->bottom_type() == TypePtr::NULL_PTR &&
> 1065:         !((in2->Opcode() == Op_ShenandoahLoadReferenceBarrier) &&
> 1066:           (((ShenandoahLoadReferenceBarrierNode*)in2)->decorators() & ON_STRONG_OOP_REF) == 0)) {

Is this `ShenandoahBarrierSet::is_strong_access`?

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

Changes requested by shade (Reviewer).

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