RFR: 8241141: Restructure humongous object allocation in G1

[Stefan Johansson]

New webrevs incorporating the new suggested search from Kim:
Full: http://cr.openjdk.java.net/~sjohanss/8241141/02/
Inc: http://cr.openjdk.java.net/~sjohanss/8241141/01-02/

Thanks,
Stefan

On 2020-04-07 10:18, Stefan Johansson wrote:
> 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.
> 
>