RFR: 8271884: G1CH::_expand_heap_after_alloc_failure is no longer needed

[Thomas Schatzl]

On Fri, 6 Aug 2021 10:43:19 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Hi,
> 
> Please review this change to remove _expand_heap_after_alloc_failure. Checks for old_is_full(), survivor_is_full(), and G1CollectedHeap::has_more_regions() should eliminate repeated expansion attempts.
> 
> Testing; tier 1

Changes requested by tschatzl (Reviewer).

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.

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

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