RFR: 8344665: Refactor PartialArrayState allocation for reuse [v3]

[Kim Barrett]

On Mon, 2 Dec 2024 11:23:53 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Kim Barrett has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - num_allocators => max_allocators
>>  - fix comment typo
>>  - use struct/union instead of constants
>
> src/hotspot/share/gc/shared/partialArrayState.hpp line 181:
> 
>> 179:   // allocator counters as a single unit for atomic manipulation.
>> 180:   using CounterValues = LP64_ONLY(uint64_t) NOT_LP64(uint32_t);
>> 181:   using Counter = LP64_ONLY(uint32_t) NOT_LP64(uint16_t);
> 
> Given the max-value has type `uint`, using the larger type on both 32/64 bit systems should be simpler and it should not cause any noticeable perf regression, since registering/releasing allocators should be infrequent. WDYT?

I assumed 16bits of worker threads was quite sufficient for a 32bit platform.
And I misremembered and thought 32bit platforms couldn't be relied upon for a
64bit atomic add and maybe other 64bit operations. And this code is definitely
not super performance critical. So yeah, I could drop the platform-conditional
definition of Counter. I don't think it makes much difference to the code.  I
guess the type aliases could be dropped and just use bare uint32/64_t.  Not
sure that's actually an improvement.

> src/hotspot/share/gc/shared/partialArrayState.hpp line 189:
> 
>> 187:   // allocators. The counters are atomic to permit concurrent construction,
>> 188:   // and to permit concurrent destruction.  They are an atomic unit to detect
>> 189:   // and reject mixing the two phases, without concern for questions of
> 
> It's nice that this library can detect and reject misuse (such as mixing two phases), but I’m not sure why so much effort was spent preventing this. None of the existing users of the library are expected to mix phases in the near future. Could we instead document that mixing two phases is not permitted, and if someone chooses to do so, they do so at their own risk?

So far, only 2 of the nearly a dozen(?) potential clients are using this. I'm
not sure that none of them are going to have workers that do some of their
setup after being started. Hence the desire to support concurrency. And if
that, then I feel better about it if there's some usage validation. But maybe
it would be better to just throw a lock at the problem. And if it turns out
none of the use-cases end up needing that concurrency, then I won't object to
a little bit of code simplification.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22287#discussion_r1866111712
PR Review Comment: https://git.openjdk.org/jdk/pull/22287#discussion_r1866111801