RFR: 8241141: Restructure humongous object allocation in G1

Mon Apr 6 19:09:14 UTC 2020

Thanks for the review Kim,

Here are updated webrevs.
Full: http://cr.openjdk.java.net/~sjohanss/8241141/01/
Inc: http://cr.openjdk.java.net/~sjohanss/8241141/00-01/

Comments below.

On 2020-04-06 08:50, Kim Barrett wrote:
>> 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.
> 
Good catch, tried out some different ways to structure the code, but in 
this version these 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.
> 
To minimize the difference from the old behavior I now pass down workers 
for the humongous case.

> 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.
Once again, good catch, protected and not virtual.

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

Mostly to match how allocate_humongous() is implemented. There might 
also be reasons to add more logic to expand_and_allocate_humongous going 
forward. One way to implement concurrent uncommit will need it. Are you 
ok with those reasons or should I make expand_and_allocate_humongous 
virtual and remove allocate_humongous_allow_expand?

> 
> ------------------------------------------------------------------------------
> 
> 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.
I updated the comment.

> 
> ------------------------------------------------------------------------------
> 
> Not part of this change, but while reviewing it I noticed that the
> expand_single_region part of G1CH::new_region seems rather contorted.
I agree, there is room for improvement here. Will you file an RFE?

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

> 
> ------------------------------------------------------------------------------
> 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.
I like the optimization in find_contiguous_in_free_list, but the 
for-loop needs to go all the way to end to check that the regions are 
really free or not committed. One could of course add a check and break 
if to close to end when encountering a used region, but not sure it is 
worth complicating the code for that. What do you think?

> 
> ------------------------------------------------------------------------------
> 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.
I agree with both.

> 
> ------------------------------------------------------------------------------
> 
> 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.
I kind of agree, but if we move forward with concurrent uncommit this 
might change to really mean available rather than committed. We probably 
want to clear the available map before really uncommitting the regions.

> 
> 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.
I agree, but we should handle this as a separate cleanup. Agree?

> 
> ------------------------------------------------------------------------------
> 