RFR: 8320525: G1: G1UpdateRemSetTrackingBeforeRebuild::distribute_marked_bytes accesses partially unloaded klass

[Thomas Schatzl]

On Fri, 24 Nov 2023 08:34:51 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1137:
>> 
>>> 1135:       // regions; also, we should not access their header any more them as their
>>> 1136:       // klass may have been unloaded.
>>> 1137:       assert(marked_bytes == 0 || cast_to_oop(hr->bottom())->size() * HeapWordSize == marked_bytes,
>> 
>> Is it possible to separate these two cases into two methods, for instance? Taking a step back, why do we even need to call `note_end_of_marking` on these effectively empty regions?
>
> We need to call `HeapRegion::note_end_of_marking` on them, even if they are empty for completeness. It's not completely necessary because reclamation will probably reset them correctly, but it's easier to reason if they (empty and nonempty regions) are handled the same to me.
> 
> I.e. so _all_ regions have `note_start/end_of_marking` called.

I do not think it is easier to understand to separate these two cases (empty/non-empty) regions here: both distributing bytes (it's fine to distribute 0 bytes) and being consistent with calling the start/end notifications (and `note_end` works as expected on empty regions too) for all regions is easier to follow to me compared to having an unnecessary exception.
Because then the question is: why have that exception?

Even if they do not do anything "meaningful" other than resetting some internal state that is later overwritten for these specially handled empty regions anyway.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16766#discussion_r1404079467