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

[Thomas Schatzl]

On Fri, 18 Sep 2020 16:08:25 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> 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.
>
>> we should never fail this expand
> 
> I think I am missing some background knowledge. I assume `expand_exact()`, in generally, could fail, but for args,
> `curr, 1, NULL` here, it will not fail, because ... The "because" part should be in the comments right above the call.
> As for the post-assert, `_regions.get_by_index(curr) != NULL && is_available(curr)`, I think they are already covered
> in `expand_exact`.
> I like `at(curr)->is_free()` better, since the current function, `find_highest_free`, should return a region (by index)
> that's free.

Looking at this again, I think the change in HeapRegionManager::find_highest_free() is not good.

While the call should never fail (it is done when allocating archive regions at VM startup when the heap is completely
empty), the caller handles that - and does not allocate the archive regions at all. Consider the use case where you
start with a small heap, but a huge CDS archive (maybe even larger than the heap). This should still continue working
without CDS usage.

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

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