RFR: 8293252: Shenandoah: ThreadMXBean synchronizer tests crash with aggressive heuristics [v2]

[William Kemper]

On Thu, 22 Sep 2022 01:59:08 GMT, Ashutosh Mehra <duke at openjdk.org> wrote:

>> Thanks for pointing out that bug. It explains the need for the keep_alive API. But that API is solving a different issue than what we are facing here. I think we are on right track with respect to adding LRB during object iteration.
>> 
>> I am going to try changing `ObjectIterateScanRootClosure` to use `HeapAccess<AS_NO_KEEPALIVE>` instead of `RawAccess`.
>> I have been deliberating over the need for `if (_heap->is_concurrent_weak_root_in_progress() && !_marking_context->is_marked(obj))` check in `ObjectIterateScanRootClosure` after we switch to HeapAccess. The same check is executed by LRB [1] if `ON_PHANTOM_OOP_REF` decorator is also set, which makes me think we should probably use `HeapAccess<AS_NO_KEEPALIVE | ON_PHANTOM_OOP_REF>` and remove that check from `ObjectIterateScanRootClosure`. wdyt?
>> 
>> [1] https://github.com/openjdk/jdk/blob/6beeb8471ccf140e59864d7f983e1d9ad741309c/src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp#L104-L108
>
> I have added commit that replaces the `RawAccess` with `HeapAccess<AS_NO_KEEPALIVE>`. I think its better not to avoid the check `if (_heap->is_concurrent_weak_root_in_progress() && !_marking_context->is_marked(obj))` by using `ON_PHANTOM_OOP_REF` decorator as it relies on the assumption that weak oops in the VM are similar to phantomly reachable references (which is true and may remain so, but is not as explicit as the condition itself). I have therefore kept the check for `is_concurrent_weak_root_in_progress` as it is.

It might be simpler to only change the one line in these object iterators:

obj = ShenandoahBarrierSet::resolve_forwarded_not_null(obj); 

to

obj = ShenandoahBarrierSet::load_reference_barrier(obj); 

This will evacuate objects in the cset after checking for unmarked concurrent weak roots. `resolve_forwarded_not_null` presumes the object has already been evacuated, but that may not be the case here.

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

PR: https://git.openjdk.org/jdk/pull/10268