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

[Roman Kennke]

On Tue, 10 Nov 2020 09:41:52 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> 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
>
> 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"?

Dunno. Could do that. Within one instance of JVM, we would never see both narrow/non-narrow versions, though.

> 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.

Yes, this is for hash/cmp. In particular, I suspect we might miss optimizations that don't find two LRBs equal only because some non-relevant access-bit is set. Will move it to 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`?

It is the same, but in my testing I found that ON_STRONG_OOP_REF is not always set (it is the default when no other ON_XYZ_OOP_REF is specified).

> 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?

No. ON_UNKNOWN_OOP_REF *might* access the referent field by reflection. We must do the correct thing in this situation. Normally, ON_UNKNOWN_OOP_REF is indeed called on normal/strong references, though, however we do the correct thing in this case too, because the referent would be marked and thus go through regular LRB. It doesn't hurt to call into weak for unknown oop refs.

> 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.

Hmm yeah. Actually I am thinking how to do the HasDecorator tests as outermost tests, because those are the compile-time-tests. Would need to duplicate the whole test once for WEAK/UNKNOWN and once for PHANTOM I guess.

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

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