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

[Kim Barrett]

On Tue, 17 Dec 2024 20:00:14 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> 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/6b515303...54c37988
>
> 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.

I think Chunk is overly generic and used a lot elsewhere.  It could just as
easily be Region (e.g. the "claimed region" instead of the "claimed chunk").
I think the "claim-ness" is the important feature here.

> 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.

This stylistic difference has been discussed at length in the past.

> 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?

I think you mean is_trivially_assignable.  I don't think it's a useful
assertion here.  Depending on details of the class, one might reasonably
implement such an operation in the same way even if it isn't trivially
assignable.

> 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.

The name log_set was chosen to suggest that it does "UL logging", and to
indicate that it is for dealing with a set of stats objects.  I think
print_stats loses both of those cues and is less clear because of that.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22622#discussion_r1890561127
PR Review Comment: https://git.openjdk.org/jdk/pull/22622#discussion_r1890561291
PR Review Comment: https://git.openjdk.org/jdk/pull/22622#discussion_r1890561350
PR Review Comment: https://git.openjdk.org/jdk/pull/22622#discussion_r1890561199