8358586: ZGC: Combine ZAllocator and ZObjectAllocator

[jsikstro]

Hello,

After JDK-8353184, ZAllocator has essentially become a mirror of ZObjectAllocator, questioning the relevance of ZAllocator. The purpose of this RFE is to combine ZAllocator and ZObjectAllocator into a single class ZObjectAllocator.

The solution I propose moves the new ZObjectAllocator as a data member in ZHeap, and is accessed solely from ZHeap. ZObjectAllocator stores a number of allocators, one for each age. Instead of storing eden and relocation allocators separately, all allocators are now in a single array, which makes it more straightforward to iterate over all allocators (when retiring pages for example). Since the underlying allocator (called PerAge) needs to be constructed with different ages, we can't use the default constructor/initialization, so I have a solution in-place for dealing with this.

Undoing an object allocation has been moved in its entirety (not calling into ZObjectAllocator) to ZHeap, where it fits much better.

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) 🔄 Re-review required (review applies to 4ef788d7)

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 is no longer ready for integration - check the PR body for details.

[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

• 07: Full - Incremental (d2b59317)
• 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.