RFR: 8241141: Restructure humongous object allocation in G1

[Stefan Johansson]

Just some comments, new webrevs later.

On 2020-04-07 09:45, Kim Barrett wrote:
>>> 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:
Nice, I'll take it for a spin.

> 
> 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.
Yes, we could probably come up with something better.

> 
>>
>>> 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.
True, but I rather than changing it here, I think it should be included 
when cleaning up G1CH::new_region which uses expand_single_region.