RFR: 8271060: Merge G1CollectedHeap::determine_start_concurrent_mark_gc and G1Policy::decide_on_conc_mark_initiation

[Albert Mingkun Yang]

On Thu, 22 Jul 2021 08:46:36 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

> Hi all,
> 
>   can I have reviews for this merge of two methods that have the same purpose: determine whether this pause should be a concurrent start pause. The reason for this merge is that `G1CollectedHeap::determine_start_concurrent_mark_gc` just calls the other, and
>   - is the only caller
>   - only adds another condition of the same type that `G1Policy::decide_on_conc_mark_initiation` does at the beginning
>   - adds some return value checking and return value
> which all imho fit into `G1Policy::decide_on_conc_mark_initiation` too.
> 
> It also reduces `G1CollectedHeap` a bit. The main reason for me to put this in the new location is basically that that additional condition in  `G1CollectedHeap::determine_start_concurrent_mark_gc` makes all conditions for that decision be located closer together.
> 
> If you think otherwise and this is a bad idea, I will close this PR.
> 
> Testing: manual gc/g1 runs
> 
> Thanks,
>   Thomas

bool should_start_concurrent_mark_operation = policy()->decide_on_concurrent_start_pause();

The name, `decide_on_concurrent_start_pause()`, is not very obvious, IMO; I feel it's because it's trying to do two things: 1. deciding the pause type, 2. returns whether the decided pause type is concurrent_start_gc.

I wonder if sth like the following (decoupling the two tasks) is clearer:


policy()->decide_pause_type();
bool should_start_concurrent_mark_operation = collector_state()->in_concurrent_start_gc();


Ofc, this is very subjective. The PR is fine as it is.

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

Marked as reviewed by ayang (Committer).

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