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

[Albert Mingkun Yang]

On Fri, 29 Nov 2024 16:09:15 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> This change splits the existing PartialArrayStateAllocator class into an
>> allocator class and a manager class.  The allocator class is per worker
>> thread.  The manager class provides the memory management context for a
>> group of allocators.
>> 
>> This change is in preparation for some other refactorings around partial array
>> state handling.  That work is intended to make it easier for various
>> collections to adopt the use of that mechanism for chunking the processing of
>> large objArrays.
>> 
>> The new implementation for the memory management context is based on the
>> existing one, with an Arena per worker, now controlled by the manager object.
>> Potential improvements to that can be explored in the future.  Some ideas
>> include improvements to the Arena API or a single thread-safe Arena variant
>> (trading slower arena allocation (which is the slow path) for less memory
>> usage).
>> 
>> G1 has a single manager, reused by each young/mixed GC. Associated state
>> allocators are nested in the per-worker structures, so deleted at the end of
>> the collection. The manager is reset at the end of the collection to allow the
>> memory to be recycled. It is planned that the STW full collector will also use
>> this manager when converted to use PartialArrayState. So it will be reused by
>> all STW collections.
>> 
>> ParallelGC has a single manager, reused by each young collection. Because the
>> per-worker promotion managers are never destroyed, their nested state
>> allocators are never destroyed. So the manager is not reset, instead leaving
>> previously allocated states in the allocator free lists for use by the next
>> collection. This means the full collector won't be able to use the same
>> manager object as the young collectors.
>> 
>> Testing: mach5 tier1-5
>
> 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

Some minor comments/suggestions.

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?

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?

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

Marked as reviewed by ayang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22287#pullrequestreview-2472436639
PR Review Comment: https://git.openjdk.org/jdk/pull/22287#discussion_r1865681792
PR Review Comment: https://git.openjdk.org/jdk/pull/22287#discussion_r1865687546