RFR: 8358483: G1: Remove G1HeapRegionManager::num_available_regions

[Thomas Schatzl]

On Tue, 3 Jun 2025 09:10:28 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> Simple cleanup of moving an API from `G1HeapRegionManager` to `G1CollectedHeap`.
> 
> Test: tier1

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_humongous`is 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. `G1HeapRegionManager`to 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.

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

PR Comment: https://git.openjdk.org/jdk/pull/25611#issuecomment-2959220545