RFR: 8345732: Provide helpers for using PartialArrayState [v4]

[Albert Mingkun Yang]

On Tue, 17 Dec 2024 18:08:23 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Please review this change that introduces two new helper classes to simplify
>> the usage of PartialArrayStates to manage splitting the processing of large
>> object arrays into parallelizable chunks.  G1 and Parallel young GCs are
>> changed to use this new mechanism.
>> 
>> PartialArrayTaskStats is used to collect and report statistics related to
>> array splitting.  It replaces the direct implementation in PSPromotionManager,
>> and is now also used by G1 young GCs.
>> 
>> PartialArraySplitter packages up most of the work involved in splitting and
>> processing tasks. It provides task allocation and release, enqueuing, chunk
>> claiming, and statistics tracking. It does this by encapsulating existing
>> objects and functionality. Using array splitting is mostly reduced to calling
>> the splitter's start function and then calling it's step function to process
>> partial states. This substantially reduces the amount of code for each client
>> to perform this work.
>> 
>> Testing: mach5 tier1-5
>> 
>> Manually ran some test programs with each of G1 and Parallel, with taskqueue
>> stats logging enabled, and checked that the logged statistics looked okay.
>
> Kim Barrett 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 14 additional commits since the last revision:
> 
>  - Merge branch 'master' into pa-splitter
>  - merge log_set decl/defn
>  - remove default counts for stats incrementers
>  - remove uneeded 'explicit'
>  - cleanup unneeded includes
>  - remove moved-from macro defines
>  - Merge branch 'master' into pa-splitter
>  - rename splitter.step() => claim()
>  - simplify comments
>  - Merge branch 'master' into pa-splitter
>  - ... and 4 more: https://git.openjdk.org/jdk/compare/999d9cc9...54c37988

Some minor suggestion.

src/hotspot/share/gc/shared/partialArraySplitter.hpp line 81:

> 79:   // Result type for claim(), carrying multiple values.  Provides the claimed
> 80:   // chunk's start and end array indices.
> 81:   struct Claim {

I feel `Chunk` is a better name.

src/hotspot/share/gc/shared/partialArraySplitter.inline.hpp line 63:

> 61: PartialArraySplitter::claim(PartialArrayState* state, Queue* queue, bool stolen) {
> 62: #if TASKQUEUE_STATS
> 63:   if (stolen) _stats.inc_stolen();

Breaking it into multiple lines make the control flow more explicit.

src/hotspot/share/gc/shared/partialArrayTaskStats.cpp line 49:

> 47: 
> 48: void PartialArrayTaskStats::reset() {
> 49:   *this = PartialArrayTaskStats();

Can we do sth like `static_assert(std::is_trivially_copyable<PartialArrayTaskStats>::value)` here?

src/hotspot/share/gc/shared/partialArrayTaskStats.hpp line 90:

> 88:   // title: A string title for the table.
> 89:   template<typename StatsAccess>
> 90:   static void log_set(uint num_stats, StatsAccess access, const char* title) {

Going through all its call sites, I believe `print_stats` is more readable.

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

Marked as reviewed by ayang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22622#pullrequestreview-2509966063
PR Review Comment: https://git.openjdk.org/jdk/pull/22622#discussion_r1889140069
PR Review Comment: https://git.openjdk.org/jdk/pull/22622#discussion_r1889142484
PR Review Comment: https://git.openjdk.org/jdk/pull/22622#discussion_r1889152438
PR Review Comment: https://git.openjdk.org/jdk/pull/22622#discussion_r1889140874