RFR: 8358586: ZGC: Combine ZAllocator and ZObjectAllocator [v2]

[Joel Sikström]

On Thu, 12 Jun 2025 09:23:35 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Joel Sikström has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - -Wconversion fixes
>>  - Intermediate alloc_object in ZHeap
>
> The change looks good. But I have a few thoughts if we want to take this a step further.
> 
> This change starts adding a static interface on `ZObjectAllocator` which is used for `retire_pages`. I wonder if we can take this all the way and make `ZObjectAllocator` a static only interface. And keep the Allocator implementation opaque / private. 
> 
> So instead of first pulling out the allocator and calling the corresponding function, we go through the static interface. So we get something like
> ```c++
>   static void initialize();
>   static void retire_pages(ZPageAgeRange range);
>   static zaddress alloc_object(size_t size, ZPageAge age);
>   static void undo_alloc_object(zaddress addr, size_t size, ZPageAge age);
>   static size_t remaining_in_eden();

Thank you for the feedback @xmas92!

I addressed your comments in a new commit. Do you think we should now rename the zObjectAllocator{.hpp, .inline.hpp, cpp} to zObjectAllocators instead, now that the static interface is the new "accessor"?

If you have any thoughts on my comment about making the ZObjectAllocator constuctor private I'm open to suggestions.

> src/hotspot/share/gc/z/zObjectAllocator.hpp line 44:
> 
>> 42:   static constexpr uint NumRelocationAllocators = NumAllocators - 1;
>> 43: 
>> 44:   typedef Deferred<ValueObjArray<ZObjectAllocator, NumAllocators>> Allocators;
> 
> Nit, I prefer `using` over `typedef`.
> Suggestion:
> 
>   using Allocators = Deferred<ValueObjArray<ZObjectAllocator, NumAllocators>>;

Sure!

> src/hotspot/share/gc/z/zObjectAllocator.hpp line 85:
> 
>> 83:   static ZObjectAllocator* eden();
>> 84: 
>> 85:   static void retire_pages(ZPageAgeRange range);
> 
> We tend to avoid mixing static and non static interfaces.
> 
> Maybe adding a `ZObjectAllocators` to wrap the static interface is preferable.

I added an AllStatic ZObjectAllocators. I think this looks good. I couldn't get around not having the ZObjectAllocator constructor being public so that ValueObjArray can access it. Adding ValueObjArray as a friend did not work either, maybe there's a better way to do this but I'm not sure.

> src/hotspot/share/gc/z/zObjectAllocator.inline.hpp line 43:
> 
>> 41:   guarantee(size <= ZHeap::heap()->max_tlab_size(), "TLAB too large");
>> 42:   return alloc_object(size);
>> 43: }
> 
> This could probably be on `ZHeap` instead and only have `alloc_object` on `ZObjectAllocator`

I agree, we don't have to include ZObjectAllocator in zCollectedHeap.cpp any more as well, and the guarantee check fits better in ZHeap instead of here.

> src/hotspot/share/gc/z/zObjectAllocator.inline.hpp line 47:
> 
>> 45: inline void ZObjectAllocator::retire_pages(ZPageAgeRange range) {
>> 46:   for (ZPageAge age : range) {
>> 47:     _allocators->at((int)untype(age))->retire_pages();
> 
> Suggestion:
> 
>     allocator(age)->retire_pages();

👍

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

PR Comment: https://git.openjdk.org/jdk/pull/25693#issuecomment-2966542623
PR Review Comment: https://git.openjdk.org/jdk/pull/25693#discussion_r2142626884
PR Review Comment: https://git.openjdk.org/jdk/pull/25693#discussion_r2142625860
PR Review Comment: https://git.openjdk.org/jdk/pull/25693#discussion_r2142621460
PR Review Comment: https://git.openjdk.org/jdk/pull/25693#discussion_r2142626528