RFR: 8304712: Only pass total number of regions into G1Policy::calc_min_old_cset_length

[Thomas Schatzl]

On Wed, 22 Mar 2023 18:24:12 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this change that refactors `G1Policy::calc_min_old_cset_length` to take only required parameters and also just use `ceil()` for the calculation. It just does not matter to be clever here imo.
>> 
>> Testing: gha
>> 
>> Thanks,
>>   Thomas
>
> src/hotspot/share/gc/g1/g1Policy.cpp line 1383:
> 
>> 1381: }
>> 1382: 
>> 1383: uint G1Policy::calc_min_old_cset_length(uint num_marked_regions) const {
> 
> I actually think the previous signature is easier to read; The name, `num_marked_regions`, doesn't tell me it's in fact the number of candidates.

Later there will be a distinction within the candidates between regions added by marking, and regions added by evacuation failure/pinning. This is a very important distinction for this method to keep mixed gcs working as they are. The comment explains it. I would prefer to not change the signature again a few PRs later.

As for the previous signature being easier to read, I think keeping the information passed to method as succinct as possible makes it easier to reason about what the method actually does (and needs), so I would like to keep it as is. We can certainly argue about the actual naming (`num_marked_regions` needs a bit of an explanation as done in the comment), but I could not find something better.
I.e. `f(int) -> int` is imo easier than `f(class with lots of stuff inside) -> int`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13147#discussion_r1145893009