RFR: 8373116: Genshen: arraycopy_work should be always done for arrays in old gen during young concurrent marking [v7]

[Kelvin Nilsen]

On Mon, 8 Dec 2025 18:09:00 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> Chasing the root cause of JDK-8372498, I have narrowed down root cause to the commit https://github.com/openjdk/jdk/commit/f8cf9ca69cfef286c80559bfe1d147b6303d10d2
>> 
>> It is caused by the behavior change from follow code:
>> 
>> Original:
>> 
>>   if (ShenandoahSATBBarrier) {
>>     T* array = dst;
>>     HeapWord* array_addr = reinterpret_cast<HeapWord*>(array);
>>     ShenandoahHeapRegion* r = _heap->heap_region_containing(array_addr);
>>     if (is_old_marking) {
>>       // Generational, old marking
>>       assert(_heap->mode()->is_generational(), "Invariant");
>>       if (r->is_old() && (array_addr < _heap->marking_context()->top_at_mark_start(r))) {
>>         arraycopy_work<T, false, false, true>(array, count);
>>       }
>>     } else if (_heap->mode()->is_generational()) {
>>       // Generational, young marking
>>       if (r->is_old() || (array_addr < _heap->marking_context()->top_at_mark_start(r))) {
>>         arraycopy_work<T, false, false, true>(array, count);
>>       }
>>     } else if (array_addr < _heap->marking_context()->top_at_mark_start(r)) {
>>       // Non-generational, marking
>>       arraycopy_work<T, false, false, true>(array, count);
>>     }
>>   }
>> 
>> New:
>> 
>>   if (ShenandoahSATBBarrier) {
>>     if (!_heap->marking_context()->allocated_after_mark_start(reinterpret_cast<HeapWord*>(dst))) {
>>       arraycopy_work<T, false, false, true>(dst, count);
>>     }
>>   }
>> 
>> 
>> 
>> With the new STAB barrier code for arraycopy_marking, if is it young GC and the array is in old region, but array is above TAMS(Old GC may not be started, TAMS of old region is not captured), arraycopy_work won't be applied anymore, so we may have missed some pointers in SATB in such case during concurrent young marking. 
>> 
>> ### Test
>> - [x] hotspot_gc_shenandoah
>> - [x] repeat gc/TestAllocHumongousFragment.java#generational and sure it won't crash with the fix
>> - [x] GHA
>
> Xiaolong Peng has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Also test is_generational

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

> 448:   assert(_heap->is_concurrent_mark_in_progress(), "only during marking");
> 449:   if (ShenandoahSATBBarrier) {
> 450:     if (!_heap->marking_context()->allocated_after_mark_start(reinterpret_cast<HeapWord*>(dst)) ||

Predicates:
  A: heap->is_concurrent_young_mark_in_progress()
  B: heap->is_concurrent_old_mark_in_progress()
  C: heap->heap_region_containining(dst)->is_old()
  D: !heap->marking_context()->allocated_after_mark_start(dst)

I think the conditions under which we need to call arraycopy_work() are:
(A && C) || (A && D) || (B && C && D)
which could be written:
(A && (C || D)) || (B && C && D)

As written, I think we are also calling arraycopy_work() under certain unnecessary conditions, such as:
(B && C)

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

> 462:             assert(!_heap->is_in_young(obj) ||
> 463:                    card_scan->is_card_dirty(elem_heap_word_ptr) ||
> 464:                    card_scan->is_write_card_dirty(elem_heap_word_ptr),

I believe there is a very slight risk of assertion failure here, which might be so rare that you could just mention the possibility in the "error message" or in a comment associated with this code.

The race is that some other thread could overwrite an an old-gen array element with pointer to young during young marking, and we might see this interesting young pointer before that other thread has had a chance to mark the associated card dirty.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28669#discussion_r2600079828
PR Review Comment: https://git.openjdk.org/jdk/pull/28669#discussion_r2600102116