RFR: 8293252: Shenandoah: ThreadMXBean synchronizer tests crash with IU+aggressive mode

[Ashutosh Mehra]

On Wed, 14 Sep 2022 19:48:54 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Please review the fix for the assertion failure when running ThreadMXBean synchronizer code in fastdebug mode.
>> 
>> Please note that the assertion failure happens in regular (satb) mode as well, not just in iu mode.
>> 
>> At the point of assertion failure, the JVM is at a safepoint as the VMThread is executing VM_ThreadDump operation and the GC worker threads are paused after the mark phase.
>> Assertion happened when the VMThread performs `NativeAccess<>::oop_store()` (as part of creating an `OopHandle`) on an object which has been marked and is in the collection set. But the failing assertion in `oop_store_not_in_heap()` expects the oop is not in the collection set.
>> 
>> The assertion is conditional on the statement `value != NULL && !ShenandoahHeap::heap()->cancelled_gc()`
>> 
>> 	shenandoah_assert_not_in_cset_if(addr, value, value != NULL && !ShenandoahHeap::heap()->cancelled_gc());
>> 
>> But it looks like it is missing another important condition - the GC should be in marking phase.
>> i.e. the assertion is valid only if it is protected by `ShenandoahHeap::heap()->is_concurrent_mark_in_progress()`.
>> So the correct assertion should be:
>> 
>> 	shenandoah_assert_not_in_cset_if(addr, value, value != NULL && !ShenandoahHeap::heap()->cancelled_gc() && ShenandoahHeap::heap()->is_concurrent_mark_in_progress());
>> 
>> In fact this assertion is already present in one of its caller (`oop_store_in_heap()1) in its negative form:
>> 
>> 	shenandoah_assert_not_in_cset_except    (addr, value, value == NULL || ShenandoahHeap::heap()->cancelled_gc() || !ShenandoahHeap::heap()->is_concurrent_mark_in_progress());
>> 
>> 
>> Also, it reads strange that `oop_store_in_heap()` would call `oop_store_not_in_heap()`.
>> So I have moved the code to a separate method `oop_store_common()` that gets called by both `oop_store_in_heap()` and `oop_store_not_in_heap()`.
>> 
>> Tested it by running following tests in fastdebug mode:
>> 
>> - hotspot_gc_shenandoah
>> - java/lang/management/ThreadMXBean/MyOwnSynchronizer.java (50 times with -XX:ShenandoahGCHeuristics=aggressive)
>> - java/lang/management/ThreadMXBean/LockedSynchronizers.java (50 times with -XX:ShenandoahGCHeuristics=aggressive)
>> 
>> Signed-off-by: Ashutosh Mehra <asmehra at redhat.com>
>
> src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 246:
> 
>> 244: inline void ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_store_common(T* addr, oop value) {
>> 245:   shenandoah_assert_marked_if(NULL, value, !CompressedOops::is_null(value) && ShenandoahHeap::heap()->is_evacuation_in_progress());
>> 246:   shenandoah_assert_not_in_cset_if(addr, value, value != NULL && !ShenandoahHeap::heap()->cancelled_gc() && ShenandoahHeap::heap()->is_concurrent_mark_in_progress());
> 
> The collection set is chosen during final mark and should always be empty during concurrent mark, so restricting this assertion to run only when concurrent mark is in progress effectively disables it.

Are you suggesting we are better off removing this assertion completely, because the assertion that the `value` is not in collection set is also not correct as is evident from this issue.

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

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