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

[Thomas Schatzl]

On Tue, 13 Sep 2022 11:49:36 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> You mean the one (ultimately) recorded in the `G1ConcurrentMark::_live_bytes` array and the one for the statistics in `G1RemoveSelfForwardsInChunksTask::process_chunk`?
>> You probably suggest to merge them somehow to do this work once too?
>> 
>> I do not think this is worth, particularly because `process_chunk` only collects live bytes per thread without any synchronization (it does collect dead bytes with synchronization).
>> So you could use that value (the total dead bytes) or vice versa to update `G1ConcurrentMark::_live_bytes` at the end of collection, but
>> * `G1ConcurrentMark::_live_bytes` (the sum) isn't actually gathered here, but again these are basically per-thread values (using the mark-stats cache), so otoh the update is inexpensive (nothing compared to the `par_mark` in `mark_evac_failure_object`), and otoh it can't be used for updating `HeapRegion::_garbage_bytes` directly. This would need to flush buffers somewhere before doing that.
>> * the collected `HeapRegion::_garbage_bytes` could be used at the end to update `G1ConcurrentMark::_live_bytes`, but it does not seem to be worth the complexity adding another phase somewhere else. Also it would require another `G1ConcurrentMark` API entry just for that which I do not see worth doing.
>> 
>> I.e. I do not think it is worth optimizing something that trivial and add the required code complexity. I would prefer to decrease code complexity.
>
> I was thinking sth like:
> 
> void HeapRegion::note_self_forwarding_removal_end_par(size_t garbage_bytes) {
>   Atomic::store(&_live_bytes, region_size - _garbage_bytes, ...);
>   Atomic::add(&_garbage_bytes, garbage_bytes, memory_order_relaxed);
> }

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 to determine the result of `HeapRegion::live_bytes()` (iirc), and its always `top() - garbage_bytes()` (see my comments in https://github.com/openjdk/jdk/pull/9511 for the 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.

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

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