RFR: 8273492: Move evacuation failure handling into G1YoungCollector [v5]

Thomas Schatzl tschatzl at openjdk.java.net
Fri Sep 24 13:08:14 UTC 2021


On Fri, 24 Sep 2021 12:55:26 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that moves young gc evacuation failure handling (`G1EvacFailureRegions`) from `G1CollectedHeap` to `G1YoungCollector`.
>> 
>> Testing: tier1-3
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   ayang review (2)

> I think the original placement about the design, update stats iff `evac_fail == false`, is better; this is sth internal to `G1Policy`, not a caller's decision. IOW, sth like:
> 
> ```c++
> G1Policy::record_young_collection_end(bool concurrent_operation_is_full_mark, bool evac_fail) {
>   // Evacuation failures skew the timing too much ...
>   update_stats = !evac_fail;
> }
> ```
> 
> The same apply to `G1Policy::record_pause`.

I agree with `record_young_collection_end`, but I am not that confident about `record_pause`. The latter is an internal API and directly controlled by the respective `record_xxx_end` methods only. However this is not a strong opinion so I changed it as well.

> 
> Not strictly part of this PR:
> 
> `num_regions_failed_evacuation` is just a getter, right? Why the name difference btw the method and the field?

I think this is coming from the collection set where the same naming (`_cur_length`) has been introduced to indicate the current length of the array used to gather elements. Should maybe be both renamed.

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

PR: https://git.openjdk.java.net/jdk/pull/5419



More information about the hotspot-gc-dev mailing list