RFR: 8030815: Code roots are not accounted for in region prediction [v2]

[Albert Mingkun Yang]

On Wed, 13 Sep 2023 08:21:26 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this change that adds accounting of code root scan to G1.
>> 
>> Previously this has not been done because code root scan has been thought of being negligible in overhead. However some recent investigations showed that it can be of significant importance.
>> 
>> The general idea of how prediction of code roots works is the same as for the (card-based) remembered set - given statistics of cost per code roots, try to predict costs for n code roots per region.
>> 
>> With this change there is some reason for confusion in the naming - we use a plain `rs_` prefix for variables containing information about the card set based remembered set, and this adds `code_root_rs` (for code roots) so the meaning of the former may not be completely clear. There is the follow up [JDK-8315848](https://bugs.openjdk.org/browse/JDK-8315848) that fixes that (https://github.com/tschatzl/jdk/tree/submit/8315848-rename-rs-prefix-to-card-rs).
>> 
>> Testing: gha, performance testing
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'master' into 8030815-code-roots-not-accounted-in-prediction
>  - 8030815 code roots not accounted

> However some recent investigations showed that it can be of significant importance.

What/how much is the effect of this better prediction when the code-root time is significant?

src/hotspot/share/gc/g1/g1Policy.cpp line 900:

> 898:                                             p->sum_thread_work_items(G1GCPhaseTimes::OptCodeRoots, G1GCPhaseTimes::CodeRootsScannedNMethods);
> 899: 
> 900:     if (total_code_roots_scanned > G1NumCodeRootsCostSampleThreshold) {

Why strictly greater than? Looking at its spec, I'd expect `>=` here.

src/hotspot/share/gc/g1/g1Policy.cpp line 904:

> 902:                                        average_time_ms(G1GCPhaseTimes::OptCodeRoots);
> 903: 
> 904:       _analytics->report_cost_per_code_root_scan_ms(avg_time_code_root_scan / total_code_roots_scanned, is_young_only_pause);

It's kind of a preexisting issue -- it's super weird to see `avg_time / total_count`; `sum_time / total_count` is more reasonable. The current calculation assumes the same number of gc-workers is used for each gc-pause, I believe.

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

PR Review: https://git.openjdk.org/jdk/pull/15632#pullrequestreview-1626884095
PR Review Comment: https://git.openjdk.org/jdk/pull/15632#discussion_r1325947319
PR Review Comment: https://git.openjdk.org/jdk/pull/15632#discussion_r1325953073