RFR: 8253286: Use expand_exact() instead of expand_at() for fixed requests in G1

[Stefan Johansson]

On Fri, 18 Sep 2020 10:52:02 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Hi all,
>> 
>> Please review this small cleanup that uses `expand_exact()` rather than `expand_at()` when requesting a specific
>> region. This will ensure that we get exactly the requested region.
>> Testing:
>> tier1-2 alone and 3-5 with other changes.
>
> src/hotspot/share/gc/g1/heapRegionManager.cpp line 466:
> 
>> 464:     HeapRegion *hr = _regions.get_by_index(curr);
>> 465:     if (hr == NULL || !is_available(curr)) {
>> 466:       expand_exact(curr, 1, NULL);
> 
> It seems that the original code handles the expansion failure case, but the new one doesn't. Better to have comments
> explaining why expansion should always success here.
> It's not covered in this PR, but the documentation for `expand_exact` function is quite abstract, which could also be
> addressed in this PR, if you feel like it.

Thanks for looking at this Albert. You are correct that this is a slight change of behavior, or at least it looks like
that, but we should never fail this expand. That's why I added the `assert()` right after, to clearly state that after
this call this region must be available. Are you thinking something like: Suggestion:

      // Region not available. Expand at this index to make it
      // available. This call should never fail.
      expand_exact(curr, 1, NULL);
      assert(_regions.get_by_index(curr) != NULL && is_available(curr),
             "Region (%u) must be available after expand", curr);
I'm not sure it adds that much but if you like this better I can certainly add it. I will also improve the assertion to
do: ++
assert(at(curr)->is_free(), "Region (%u) must be available and free after expand", curr);
The call to at() will do the other asserts and checking that the regions is free is a good improvement.

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

PR: https://git.openjdk.java.net/jdk/pull/235