8358483: G1: Remove G1HeapRegionManager::num_available_regions

[albertnetymk]

Simple cleanup of moving an API from G1HeapRegionManager to G1CollectedHeap.

Test: tier1

---

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-8358483: G1: Remove G1HeapRegionManager::num_available_regions (Enhancement - P4)

Reviewers

• Thomas Schatzl (@tschatzl - Reviewer)
• Sangheon Kim (@sangheon - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25611

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back ayang! 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]

@albertnetymk 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:

8358483: G1: Remove G1HeapRegionManager::num_available_regions

Reviewed-by: tschatzl, sangheki

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 33 new commits pushed to the master branch:

• 523a4ef: 8358970: CounterMonitorMBean.getDerivedGaugeTimeStamp() JavaDoc incorrectly documents null
• 0dd7c69: 8357286: (bf) Remove obsolete instanceof checks in CharBuffer.append
• 66535fe: 8358701: Remove misleading javax.management.remote API doc wording about JMX spec, and historic link to JMXMP
• ... and 30 more: https://git.openjdk.org/jdk/compare/5ae32c4c86916120d2a337be9765cb6e14f3f443...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]

@albertnetymk 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

• 01: Full - Incremental (0f3b7e76)
• 00: Full (396dbd57)

[walulyai]

I would advocate for moving more APIs into G1HeapRegionManager instead of adding more to G1CollectedHeap.

[sangheon]

I agree with @walulyai .
Adding convenient APIs on G1CollectedHeap is a matter of preference. But I dislike removing a basic API from G1HeapRegionManager. Maybe we disagree whether num_available_regions() is a basic API or not?

[albertnetymk]

Perhaps we disagree on whether num_available_regions() should be considered a basic API.

Thank you for the term; I was having a difficult time finding an accurate word for it. Since its implementation involves calculations based on other counters, I tend to view it as a "compound" API -- better suited at the level of the owner of G1HeapRegionManager rather than being part of its core interface. Hence, the suggestion in this PR.

(There are other "compound" APIs, e.g. uint num_used_regions() const { return num_committed_regions() - num_free_regions(); }, which I think also belongs to G1CollectedHeap.)

[sangheon]

I like the term 'compound' API, but still I cannot fully agree with you, @albertnetymk. e.g. if there is other user of G1HeapRegionManager, the new one also needs to implement such compound API again.
I do not want to block you if others agree with this change.

[walulyai]

Is there any benefit to implementing these helper functions for _hrm in G1CollectedHeap instead of having callers directly call the methods in _hrm through an accesser G1CollectedHeap::hrm()? I see thatwe implement many helper functions that just call similarly named methods on _hrm.

[tschatzl]

I think the idea of this change is okay; the change does not add additional API, just removes one from G1HeapRegionManager. Keeping the G1HeapRegionManager interface minimal is desirable.

The question rather seems to be who should implement that early exit in G1HeapRegionManager::find_contiguous_allow_expand(): foregoing the question whether putting this into G1CollectedHeap is better for now, but if one needs num_available_regions in G1HeapRegionManager anyway, it seems better to keep it there and just forward it to G1CollectedHeap instead of code duplication.

Trying to answer the question about the right place for that optimization: To mirror other code like allocate_humongous() which also has the special casing to avoid some expensive operation in G1HeapRegionManager, I would tend to keep the check there, and so my preference would be to keep the code as is. That special-casing does not fit in the suggested location too imo.
The expected outcome of the call to expand_and_allocate_humongousis some expensive memory operation too.

The situation for num_used_regions() is different, there is no benefit in having it in G1HeapRegionManager afaics, apart from maybe interface completeness.

Is there any benefit to implementing these helper functions for _hrm in G1CollectedHeap instead of having callers directly call the methods in _hrm through an accesser G1CollectedHeap::hrm()? I see that we implement many helper functions that just call similarly named methods on _hrm.

In general though, I think we should not expose G1HeapRegionManager to other classes via the suggested hrm() getter because imo the region management (getting information about them, iteration) seems to be fairly central to the heap. G1HeapRegionManagerto me is an integral part of the CollectedHeap, and only factored out into an extra class for convenience.

E.g. just like getting information about survivor/eden/old region numbers (e.g. eden_regions_count()), it would be a hassle to make everyone call different classes (e.g. G1EdenRegions for the example) to get such basic heap information.

What might improve the interface would be moving lots of these trivial forwarding/compound methods into the respective .inline.hpp file.

[walulyai]

In general though, I think we should not expose G1HeapRegionManager to other classes via the suggested hrm() getter because imo the region management (getting information about them, iteration) seems to be fairly central to the heap. G1HeapRegionManagerto me is an integral part of the CollectedHeap, and only factored out into an extra class for convenience.

If this is the general approach, then it is fine by me.

[albertnetymk]

Trying to answer the question about the right place for that optimization: To mirror other code like allocate_humongous() which also has the special casing to avoid some expensive operation in G1HeapRegionManager, I would tend to keep the check there, and so my preference would be to keep the code as is. That special-casing does not fit in the suggested location too imo.

On rereading the surrounding code, the early-return logic can even precede the following, right?

// early-return based on obj_regions vs num_available_regions
G1HeapRegion* humongous_start = _hrm.allocate_humongous(obj_regions);

[tschatzl]

On rereading the surrounding code, the early-return logic can even precede the following, right?

// early-return based on obj_regions vs num_available_regions
G1HeapRegion* humongous_start = _hrm.allocate_humongous(obj_regions);

I think so too; this can be an argument for moving this out; an alternative that seems to be better could be moving all the code related to G1HeapRegionManager in this method into it. Both allocate_humongous and expand_and_allocate_humongous are only called in this one method of G1CollectedHeap.

Then the (public) interface for G1HeapRegionManager could be even smaller; there also seems to be some opportunity to flatten out a few methods in G1HeapRegionManager along the way.

[albertnetymk]

an alternative that seems to be better could be moving all the code related to G1HeapRegionManager in this method into it.

If I understand you correctly, the suggestion is to merge expand_and_allocate_humongous into allocate_humongous. However, in that case the caller (g1-heap) doesn't know whether expansion happened or not. (The heap needs to call record_new_heap_size when expanding.)

  G1HeapRegion* humongous_start = _hrm.allocate_humongous(obj_regions);
  if (humongous_start == nullptr) {
    humongous_start = _hrm.expand_and_allocate_humongous(obj_regions);
    if (humongous_start != nullptr) {
      policy()->record_new_heap_size(num_committed_regions());

[tschatzl]

an alternative that seems to be better could be moving all the code related to G1HeapRegionManager in this method into it.

If I understand you correctly, the suggestion is to merge expand_and_allocate_humongous into allocate_humongous. However, in that case the caller (g1-heap) doesn't know whether expansion happened or not. (The heap needs to call record_new_heap_size when expanding.)

  G1HeapRegion* humongous_start = _hrm.allocate_humongous(obj_regions);
  if (humongous_start == nullptr) {
    humongous_start = _hrm.expand_and_allocate_humongous(obj_regions);
    if (humongous_start != nullptr) {
      policy()->record_new_heap_size(num_committed_regions());

An additional return parameter could handle that, like I prototyped at 5cf6f25 . However overall I think the change as is is okay.

[albertnetymk]

Thanks for review.

/integrate

[openjdk]

Going to push as commit 032ead1.
Since your change was applied there have been 38 commits pushed to the master branch:

• a8b4284: 8359293: Make TestNoNULL extensible
• ed39e17: 8359200: Memory corruption in MStack::push
• 6749c62: 8358772: Template-Framework Library: Primitive Types
• ... and 35 more: https://git.openjdk.org/jdk/compare/5ae32c4c86916120d2a337be9765cb6e14f3f443...master

Your commit was automatically rebased without conflicts.

[openjdk]

@albertnetymk Pushed as commit 032ead1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.