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

[Thomas Schatzl]

Hi Stefan,

On 18.09.20 21:56, Stefan Johansson wrote:
> On Fri, 18 Sep 2020 16:22:00 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:
> 
>>>> 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.
> 
> @tschatzl, not sure I follow the use case you describe and how using `expand_exact()` will differ from `expand_at()`.
> Since we're searching backwards in `HeapRegionManager::find_highest_free()` we know `expand_at()` can only expand
> precisely the requested region (all potentially higher regions must be in use already). If for some reason
> `expand_at()` would have expanded another region in the old code, this would be a bug, because we return the requested
> index.
> 

   I have been concerned about the commit potentially failing, and the 
result of that not being passed on to the caller. However, if a commit 
fails, we exit the VM anyway. So I'll retract that objection and mark it 
as reviewed.

Thomas