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