RFR: 8351091: Shenandoah: global marking context completeness is not accurately maintained

[Xiaolong Peng]

On Tue, 4 Mar 2025 18:14:58 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2837:
>> 
>>> 2835:   } else if (affiliation == OLD_GENERATION) {
>>> 2836:     return old_generation();
>>> 2837:   } else if (affiliation == FREE) {
>> 
>> I don't think it makes sense to connect `FREE` regions to the global generation in this way. Free regions are _not_ affiliated with any generation. I think in some of these cases where you want to find the mark context, it would be possible to take it from a `_generation` member variable.
>
> Yeah, I don't think it is necessary to change the behavior here either, I'll remove it in later update.

I have removed the change.

>> src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp line 337:
>> 
>>> 335:   // If generation is young and referent is in old, marking context of the old
>>> 336:   // may or may not be complete, we can safely drop the reference when old gen mark is complete.
>>> 337:   if (_generation->is_young() && referent_region->is_old()) {
>> 
>> Have you seen this happen? The reference processor for each generation is only supposed to discover references for which the referent is in the collected generation. See `ShenandoahReferenceProcessor::should_discover`:
>> 
>>   if (!heap->is_in_active_generation(referent)) {
>>     log_trace(gc,ref)("Referent outside of active generation: " PTR_FORMAT, p2i(referent));
>>     return false;
>>   }
>
> Ok, I didn't see happen in any of the jtreg tests yet.
> 
> Just base on the the behavior we saw in old gc, I assumed this could happen. Now I am more curious about the real cause of the crash caused by reference from old to young, since we always check if the referent is in the active generation, that shouldn't have happened if it works as described, my feeling is there might be something fishy in the place where we use `_active_generation`(the comments it should be update only in the STW phases), maybe should we should get rid of it, currently we directly use _gc_generation in many places as well, not sure it if is possible to cause inconsistency.
> 
> I'll revert this part, I'll follow up on the questions in separate work.

Reverted, thanks!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1980219344
PR Review Comment: https://git.openjdk.org/jdk/pull/23886#discussion_r1980239633