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

[Thomas Schatzl]

On Thu, 23 Nov 2023 14:19:47 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this fix that removes the access to a partially unloaded (i.e. unlinked only) `Klass` used for debug code in `G1UpdateRemSetTrackingBeforeRebuild::distribute_marked_bytes`.
>> 
>> This starts to fail if metadata purging happens before the call to this methods (as https://bugs.openjdk.org/browse/JDK-8317809 suggests). The test gc/g1/humongousObjects/TestHumongousClassLoader.java starts to crash on linux-x86 with 100% reproduction because it more aggressively uncommits memory when purging metaspace.
>> 
>> The fix fixes the asserts to only access the klass when it should not be unloaded yet.
>> 
>> Testing: failing test case not failing any more, gha
>> 
>> Thanks,
>>   Thomas
>
> 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.

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

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