RFR: 8256265: G1: Improve parallelism in regions that failed evacuation [v2]

[Thomas Schatzl]

On Tue, 13 Sep 2022 13:54:05 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Live-bytes are required to be updated for the marking, i.e. within the concurrent start pause. `HeapRegion` never had `_live_bytes`, only `_marked_bytes` which you probably meant.
>> 
>> `_marked_bytes` has been removed in  [JDK-8290357](https://bugs.openjdk.org/browse/JDK-8290357) because it's not required to actually store that value. It has only ever been used for some unnecessary reasons (see my comments in https://github.com/openjdk/jdk/pull/9511 for the removal reasons). 
>> 
>> At the end of marking the marked bytes (or actually `HeapRegion::_garbage_bytes`) are set by the `Remark` pause based on its `live/marked_bytes`. So we need to update the marking's `_live_bytes` here so that the values add up in the end (without using extra storage somewhere).
>> 
>> Actually I think now that we do not strictly need to update `HeapRegion::_garbage_bytes` in evacuation failure handling; updating the value for the marking is sufficient as we only use it (apart from some logging) for determining the region's gc efficiency. That one is strictly using the information from the latest marking (just completed), so that update isn't really necessary. I can remove this if wanted, but if so I'd prefer to do that separately.
>
> I see; I was assuming `_live_bytes` and `_garbage_bytes` live in the same class/context/entity. In current impl, `_live_bytes` lives in CM while `_garbage_bytes` in `HeapRegion`, which is quite confusing IMO.

There has never been a `_live_bytes` as implied by the name anywhere (there is a getter on `HeapRegion` that (still) calculates an upper bound on the number of live bytes in a region; it uses `_garbage_bytes` to determine that). The use of `_live_bytes` is maybe a misnomer in the marking code (but for marking only something below TAMS is relevant).

I am open to renaming it (separately).

In `HeapRegion` the name of this counter has always been `_marked_bytes` which meant "marked bytes below TAMS". TAMS is something that is only useful (and valid) during marking, a `_marked_bytes` on `HeapRegion` is imho very questionable (because `tams == bottom` most of the time). I.e. having nonzero marked bytes in `[bottom, tams[`  where `tams == bottom` is even more inconsistent.

Marked bytes for a region only has meaning with an associated TAMS. That's why it's gone, and it is unnecessary to store in the first place as the removal change showed.

Garbage bytes is the lower bound of dead objects between [bottom, top[, which has meaning regardless of other temporary marking data like TAMS.

This is one reason I am suggesting to (also) move TAMS out of `HeapRegion`. I only wanted to wait with that until after this change; another that TAMS is intrinsically linked to the bitmap (and marking) so it should be managed (like TARS) by `G1ConcurrentMark`, and hence cleared with the bitmap in one go.

That G1 messes with the bitmap for evacuation failure handling is from an abstraction POV not good, but overall acceptable.

Maybe the `clear_bitmap_for_region()` method should (already) be called something like `clear_marking_data_in_region()`?

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

PR: https://git.openjdk.org/jdk/pull/9980