RFR: 8271884: G1CH::_expand_heap_after_alloc_failure is no longer needed [v2]

[Ivan Walulya]

On Tue, 10 Aug 2021 20:54:49 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 167:
>> 
>>> 165:   HeapRegion* res = _hrm.allocate_free_region(type, node_index);
>>> 166: 
>>> 167:   if (res == NULL && do_expand) {
>> 
>> Ivan and me were looking into any adverse impact when removing this, in particular the expansion being called very often without `_expand_heap_after_alloc_failure`.
>> There is indeed an issue with convoying in both the survivor and old gen region allocation; the code that ultimately calls down here looks like the following:
>> 
>>   if (result == NULL && !old_is_full()) {
>>     MutexLocker x(FreeList_lock, Mutex::_no_safepoint_check_flag);
>>     result = old_gc_alloc_region()->attempt_allocation_locked(min_word_size,
>>                                                               desired_word_size,
>>                                                               actual_word_size);
>>     if (result == NULL) {
>>       set_old_full();
>>     }
>>   }
>>   ```
>> (likewise for survivor allocation)
>> After the first check to `old_is_full()` we get the `FreeList_lock` and after that call down to the allocation. This is actually the problem: multiple threads can be waiting for the lock trying to allocate, and all of these waiters will be able to try to expand without that `_expand_heap_after_alloc_failure` flag.
>> The solution is to check `old_is_full()` after grabbing the lock again - which is actually better than only finding out here quite late that there is nothing to allocate.
>
> We should probably check if there is a similar convoying effect for mutator allocation, possibly in a separate CR.

Thanks @tschatzl  for the detailed explanation. Fixed!

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

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