RFR: 8241141: Restructure humongous object allocation in G1

[Kim Barrett]

> On Apr 6, 2020, at 3:09 PM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
> 
> 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.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?

OK, leave it as is.

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

OK.

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

I did a poor job of describing what I meant, but you seem to have
figured it out. As you surmised, if candidate > (end - num) then
there's no need to continue, just return failure, and that's true both
initially and each time candidate gets updated. So maybe I steered you
wrong with the suggestion that find_contiguous_in_free_list should
make that check directly, and it would be better to make that check in
find_contiguous_in_range each time candidate gets initialized or
updated.  I don't think that really complicates the code; not having
something like that caused me to spend some time trying to understand
if there was a reason for not having the check I expected to find that
the [candidate, candidate + num) sequence would fit.

But there's another improvement for find_contiguous_in_range that could
be made. The current version checks every region from start until the
end of the successful candidate. If the iteration to check the regions
of a candidate is performed from the end, then leading regions of a
rejected candidate may not be examined at all. To avoid rescanning the
tail of the previously rejected candidate we need to keep track of that
too.

Here's a (untested!) version of that approach:

uint find_contiguous_in_range(uint start, uint end, uint num) {
  assert(start <= end, "precondition");
  assert(num >= 1, "precondition");
  uint candidate = start;       // First region in candidate sequence.
  uint unchecked = candidate;   // First unchecked region in candidate.
  // While the candidate sequence fits in the range...
  while (num <= (end - candidate)) {
    // Walk backward over the regions for the current candidate.
    for (uint i = candidate + num - 1; true; --i) {
      if (is_available(i) && !at(i)->is_free()) {
        // Region i can't be used, so restart with i+1 as the start
        // of a new candidate sequence, and with the region after the
        // old candidate sequence being the first unchecked region.
        unchecked = candidate + num;
        candidate = i + 1;
        break;
      } else if (i == unchecked) {
        // All regions of candidate sequence have passed check.
        guarantee_contiguous_range(candidate, num);
        return candidate;
      }
    }
  }
  return G1_NO_HRM_INDEX;
}

Depending on the distribution of usable vs unusable regions and
(especially) the size of num, this might examine many fewer regions.

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

OK, so “committed” might not be the right term, but “available” doesn’t have
the right connotation either, at least for me.  I don’t have a better suggestion
just now though.

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

Changing the name doesn't seem worth a separate change.