8358586: ZGC: Combine ZAllocator and ZObjectAllocator

[jsikstro]

Hello,

The main purpose of this RFE is to merge the ZAllocator class into ZObjectAllocator. After JDK-8353184, ZAllocator has essentially become a mirror of ZObjectAllocator, with a few tweaks for different behavior for eden or relocation allocations. The goal is to make the code a bit easier to follow and generalise the different paths for eden and relocation allocations to a shared path.

The solution I propose adds a static-only interface called ZObjectAllocator, which interfaces with a number of statically allocated allocators of the class ZObjectAllocatorImpl. The ZObjectAllocatorImpl class is the combination of ZAllocator and ZObjectAllocator. The initialization of the static allocators is done via ZInitialize (calling into ZObjectAllocator::initialize()) instead of in the ZHeap constructor.

Instead of storing eden and relocation allocators separately, all allocators are now in a single array, which makes over all allocators more straightforward (when retiring pages for example). The allocators are only used in ZObjectAllocator.cpp, and can only be accessed from there, or externally via the static functions in ZObjectAllocator.

allob_object and alloc_tlab now have intermediate calls with checks in ZHeap, so that ZObjectAllocator only exposes an alloc_object. This makes the API in ZObjectAllocator very clean and straightforward to understand. As a side note, I've made ZHeap::out_of_memory const so that it can be called from ZHeap::alloc_object, which is also const.

The new ZObjectAllocator now stores two static constants: NumAllocators and NumRelocationAllocators, which replaces ZAllocator::_relocation_allocators. I plan on moving NumRelocationAllocators to a more sensible place in a future patch.

Testing:

• Oracle's tier 1-5

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8358586: ZGC: Combine ZAllocator and ZObjectAllocator (Enhancement - P4)

Reviewers

• Axel Boldt-Christmas (@xmas92 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25693/head:pull/25693
$ git checkout pull/25693

Update a local copy of the PR:
$ git checkout pull/25693
$ git pull https://git.openjdk.org/jdk.git pull/25693/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25693

View PR using the GUI difftool:
$ git pr show -t 25693

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25693.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jsikstro! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@jsikstro This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8358586: ZGC: Combine ZAllocator and ZObjectAllocator

Reviewed-by: aboldtch

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 89 new commits pushed to the master branch:

• 9d06057: 8358892: RISC-V: jvm crash when running dacapo sunflow after JDK-8352504
• fedd0a0: 8359248: JFR: Help text for-XX:StartFlightRecording:report-on-exit should explain option can be repeated
• 79497ef: 8348574: Simplify c1/c2_globals inclusions
• ... and 86 more: https://git.openjdk.org/jdk/compare/1c72b350e417bd210dc6b66eba5afe0e3bc6306c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@jsikstro The following label will be automatically applied to this pull request:

• hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 06: Full - Incremental (4ef788d7)
• 05: Full - Incremental (1885d763)
• 04: Full - Incremental (54af2098)
• 03: Full - Incremental (12a3b140)
• 02: Full - Incremental (9412bae5)
• 01: Full - Incremental (4d33d429)
• 00: Full (24489a20)

[jsikstro]

I addressed some offline feedback from @xmas92 on keeping the callsite in ZCollectedHeap clean and delegating the actual work (the null-check and out-of-memory accounting) to ZHeap.

I also fixed some -Wconversion warnings in zObjectAllocator.inline.hpp.

[xmas92]

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

  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();

[jsikstro]

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

[jsikstro]

I got it! I friend class'd ValueObjBlock, which actually calls the constructor, not ValueObjArray. We can't specialize on the Count, since ValueObjBlock decrements it when constructing the array, and we can't do partial specialization, so I ended up friend class'ing any specialization of ValueObjBlock.

/Users/jsikstro/dev/jdk/open/src/hotspot/share/gc/z/zObjectAllocator.hpp:43:10: error: partial specialization cannot be declared as a friend
  friend class ValueObjBlock<ZObjectAllocator, Count>;

I think I prefer this over keeping the constructor of ZObjectAllocator public, which now allows us to make the entire implementation of ZObjectAllocator opaque/private.

[jsikstro]

With some more feedback from @xmas92 I have moved the implementation of ZObjectAllocator to the .cpp file, now named ZObjectAllocatorImpl. I have also moved the storage of the static allocators to the .cpp file and the .hpp file only contains the static interface, now called ZObjectAllocator.

[xmas92]

lgtm.

[jsikstro]

FYI: I updated the description of the PR to match the latest changes.