RFR: 8241141: Restructure humongous object allocation in G1

[Kim Barrett]

> On Mar 19, 2020, at 11:17 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
> 
> Hi,
> 
> Please review this refactoring of the humongous allocation code in G1.
> 
> Issue: https://bugs.openjdk.java.net/browse/JDK-8241141
> Webrev: http://cr.openjdk.java.net/~sjohanss/8241141/00/
> 
> Summary
> The allocation code for humongous objects is scattered between G1CollectedHeap and the HeapRegionManager. This change moves the allocating regions part into the manager, while heap is still responsible for initializing the object. The code that finds contiguous regions in the heap has also been refactored a bit to simplify the logic. Now we make sure that we only search among available regions when not wanting to expand the heap.
> 
> Testing
> Tier 1-5 in Mach5 and local stress testing.
> 
> Thanks,
> Stefan

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.hpp
 196   virtual HeapRegion* allocate_humongous_from_free_list(uint regions);
 197   virtual HeapRegion* allocate_humongous_allow_expand(uint regions);

These appear to be extension points for derived classes, and not
intended to be public APIs, so should not be public.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.hpp
 248   // Used for humongous regions, to expand exactly the requested range.
 249   virtual void expand_exact(uint start, uint num_regions, WorkGang* pretuoch_workers);

This has a "pretuoch_workers" (note misspelling) argument, but all
callers seem to pass NULL.

This also seems to be an extension point for derived classes and not
intended to be a public API. Except that while it is declared virtual
there is only the one definition.  So not an extension point, but
rather a helper function also available to derived classes.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.cpp
 163 HeapRegion* HeapRegionManager::expand_and_allocate_humongous(uint regions) {
 164   return allocate_humongous_allow_expand(regions);
 165 }

This is the only call to allocate_humongous_allow_expand. So
expand_and_allocate_humongous is just a non-virtual forwarding
function for the virtual allocate_xxx. Since the Non-Virtual Interface
pattern isn't much used in HotSpot, and isn't really needed in this
kind of situation, I wonder why this is being done here.

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

Not part of this change, but while reviewing it I noticed an
inconsistency between the description and the implementation / usage
of FreeRegionList::remove_starting_at.  The description says
Num_regions must be > 1, but in fact it must be >= 1, and that's
what's asserted by it.

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

Not part of this change, but while reviewing it I noticed that the
expand_single_region part of G1CH::new_region seems rather contorted.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.cpp
 347   // or not available so that we can make the availabel and use them.

the availabel => them available

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.cpp 
 360   for (uint i = start; i < end; i++) {

Can subtract (num - 1) from end for iteration bound.  But need to be
careful about underflow.  But maybe underflow should be the
responsibility of the caller?  That is, the caller should ensure
end - start >= num?  That check would probably speed up
find_contiguous_in_free_list.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegionManager.cpp 
 387   } while (range_start < max_length() && candidate == G1_NO_HRM_INDEX);

Shouldn't that be range_end rather than range_start?  I think
range_start performs an extra useless iteration.

I also think the candidate check should be first.

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

Not part of this change, but I found the name of
HeapRegionManager::is_available() pretty confusing, especially as used
in expand_on_preferred_node.  The "available" nomenclature generally
seems confusing to me; why isn't it "committed", since that seems to
be what it means.

Also, I think the preferred_node parameter name in expand_on_preferred_node
is confusing, since it's not obvious that it's a node index, and region
indexes abound in its vicinity.

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