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

[Thomas Schatzl]

On Fri, 24 Nov 2023 12:46:32 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> > so all regions have note_start/end_of_marking called.
> 
> Currently, the pair is invoked on all regions and the filtering (skipping young-region for example) is done inside these methods. However, the logic why a particular kind of region can (should) be skipped really belongs to the caller. The region itself doesn't know how to react to marking-start/end. (This is kind of tied to the ticket of moving marking-related fields outside region.)

We already agreed that these `note*` methods are basically part of the caller, placed in the wrong location because the members it accesses are in the wrong location. I do not think messing with this here in this CR half-heartedly is a good idea. As soon as the work to move TAMS and PB starts, this is going to change anyway and is imho a more appropriate time to reconsider this (and will probably naturally fix itself).

The problematic code is assertion code, which quite often accesses internals one normally would not. The regular code is independent of whether the region's klass is live or not after all.

Deleting this assert would fix the issue at hand as well. Another option would be to just not do class unloading this early; there is no particular reason to do it right after marking completed.

> 
> > why have that exception?
> 
> Because live-region and effective-region are diff, and mixing them causes confusion.

What is an "effective-region" in this context? I do not understand this sentence.

>I think the existence of the new comment "we should not access their header any more them..." demonstrates that it's not super obvious why the current code (before this PR) is problematic.

To me this change indicates that the sanity check code (this problematic statement is part of sanity check code - regular code does not use it) is doing things it should not. The original author (me I guess) correctly considered that we already unloaded classes super-early for some reason, wanted to have some extra check there, but then botched the refactoring (factoring out the `obj_size_in_words` calculation for the two uses).
That particular new comment is only to make it abundantly clear to not factor out any kind of `obj_size` calculation any more (I removed the second use in this change).
Maybe not adding the comment would have been better, as the `marked_bytes == 0` predicate already indicates just that (and the second use of `obj_size_in_words` is gone)

Looking at the code again, another source for confusion is maybe wrong comment placement. I will improve these.

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

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