8192647: GClocker induced GCs can starve threads requiring memory leading to OOME

Reviewed-by: tschatzl, iwalulya, egahlin

Author: albertnetymk

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -1873,7 +1873,7 @@ bool G1CollectedHeap::try_collect(GCCause::Cause cause,
     return try_collect_concurrently(cause,
                                     counters_before.total_collections(),
                                     counters_before.old_marking_cycles_started());
-  } else if (cause == GCCause::_gc_locker || cause == GCCause::_wb_young_gc
+  } else if (cause == GCCause::_wb_young_gc
              DEBUG_ONLY(|| cause == GCCause::_scavenge_alot)) {
 
     // Schedule a standard evacuation pause. We're setting word_size

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp

@@ -188,6 +188,7 @@ void ParallelScavengeHeap::post_initialize() {
   PSPromotionManager::initialize();
 
   ScavengableNMethods::initialize(&_is_scavengable);
+  GCLocker::initialize();
 }
 
 void ParallelScavengeHeap::update_counters() {
@@ -288,7 +289,6 @@ HeapWord* ParallelScavengeHeap::mem_allocate_work(size_t size,
 
   uint loop_count = 0;
   uint gc_count = 0;
-  uint gclocker_stalled_count = 0;
 
   while (result == nullptr) {
     // We don't want to have multiple collections for a single filled generation.
@@ -318,37 +318,10 @@ HeapWord* ParallelScavengeHeap::mem_allocate_work(size_t size,
           return result;
         }
       }
-
-      if (gclocker_stalled_count > GCLockerRetryAllocationCount) {
-        return nullptr;
-      }
-
-      // Failed to allocate without a gc.
-      if (GCLocker::is_active_and_needs_gc()) {
-        // If this thread is not in a jni critical section, we stall
-        // the requestor until the critical section has cleared and
-        // GC allowed. When the critical section clears, a GC is
-        // initiated by the last thread exiting the critical section; so
-        // we retry the allocation sequence from the beginning of the loop,
-        // rather than causing more, now probably unnecessary, GC attempts.
-        JavaThread* jthr = JavaThread::current();
-        if (!jthr->in_critical()) {
-          MutexUnlocker mul(Heap_lock);
-          GCLocker::stall_until_clear();
-          gclocker_stalled_count += 1;
-          continue;
-        } else {
-          if (CheckJNICalls) {
-            fatal("Possible deadlock due to allocating while"
-                  " in jni critical section");
-          }
-          return nullptr;
-        }
-      }
     }
 
-    if (result == nullptr) {
-      // Generate a VM operation
+    assert(result == nullptr, "inv");
+    {
       VM_ParallelCollectForAllocation op(size, is_tlab, gc_count);
       VMThread::execute(&op);
 
@@ -358,13 +331,6 @@ HeapWord* ParallelScavengeHeap::mem_allocate_work(size_t size,
       if (op.prologue_succeeded()) {
         assert(is_in_or_null(op.result()), "result not in heap");
 
-        // If GC was locked out during VM operation then retry allocation
-        // and/or stall as necessary.
-        if (op.gc_locked()) {
-          assert(op.result() == nullptr, "must be null if gc_locked() is true");
-          continue;  // retry and/or stall as necessary
-        }
-
         // Exit the loop if the gc time limit has been exceeded.
         // The allocation must have failed above ("result" guarding
         // this path is null) and the most recent collection has exceeded the
@@ -416,18 +382,15 @@ HeapWord* ParallelScavengeHeap::allocate_old_gen_and_record(size_t size) {
 }
 
 HeapWord* ParallelScavengeHeap::mem_allocate_old_gen(size_t size) {
-  if (!should_alloc_in_eden(size) || GCLocker::is_active_and_needs_gc()) {
-    // Size is too big for eden, or gc is locked out.
+  if (!should_alloc_in_eden(size)) {
+    // Size is too big for eden.
     return allocate_old_gen_and_record(size);
   }
 
   return nullptr;
 }
 
 void ParallelScavengeHeap::do_full_collection(bool clear_all_soft_refs) {
-  if (GCLocker::check_active_before_gc()) {
-    return;
-  }
   PSParallelCompact::invoke(clear_all_soft_refs);
 }
 
@@ -446,11 +409,6 @@ HeapWord* ParallelScavengeHeap::satisfy_failed_allocation(size_t size, bool is_t
 
   HeapWord* result = nullptr;
 
-  GCLocker::check_active_before_gc();
-  if (GCLocker::is_active_and_needs_gc()) {
-    return expand_heap_and_allocate(size, is_tlab);
-  }
-
   // If young-gen can handle this allocation, attempt young-gc firstly.
   bool should_run_young_gc = is_tlab || should_alloc_in_eden(size);
   collect_at_safepoint(!should_run_young_gc);
@@ -544,10 +502,6 @@ void ParallelScavengeHeap::collect(GCCause::Cause cause) {
     full_gc_count = total_full_collections();
   }
 
-  if (GCLocker::should_discard(cause, gc_count)) {
-    return;
-  }
-
   while (true) {
     VM_ParallelGCCollect op(gc_count, full_gc_count, cause);
     VMThread::execute(&op);
@@ -562,20 +516,7 @@ void ParallelScavengeHeap::collect(GCCause::Cause cause) {
         return;
       }
     }
-
-    if (GCLocker::is_active_and_needs_gc()) {
-      // If GCLocker is active, wait until clear before retrying.
-      GCLocker::stall_until_clear();
-    }
-  }
-}
-
-void ParallelScavengeHeap::try_collect_at_safepoint(bool full) {
-  assert(SafepointSynchronize::is_at_safepoint(), "precondition");
-  if (GCLocker::check_active_before_gc()) {
-    return;
   }
-  collect_at_safepoint(full);
 }
 
 bool ParallelScavengeHeap::must_clear_all_soft_refs() {
@@ -889,11 +830,11 @@ GrowableArray<MemoryPool*> ParallelScavengeHeap::memory_pools() {
 }
 
 void ParallelScavengeHeap::pin_object(JavaThread* thread, oop obj) {
-  GCLocker::lock_critical(thread);
+  GCLocker::enter(thread);
 }
 
 void ParallelScavengeHeap::unpin_object(JavaThread* thread, oop obj) {
-  GCLocker::unlock_critical(thread);
+  GCLocker::exit(thread);
 }
 
 void ParallelScavengeHeap::update_parallel_worker_threads_cpu_time() {

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp

@@ -95,8 +95,6 @@ class ParallelScavengeHeap : public CollectedHeap {
 
   void update_parallel_worker_threads_cpu_time();
 
-  void collect_at_safepoint(bool full);
-
   bool must_clear_all_soft_refs();
 
   HeapWord* allocate_new_tlab(size_t min_size, size_t requested_size, size_t* actual_size) override;
@@ -198,7 +196,7 @@ class ParallelScavengeHeap : public CollectedHeap {
   // Support for System.gc()
   void collect(GCCause::Cause cause) override;
 
-  void try_collect_at_safepoint(bool full);
+  void collect_at_safepoint(bool full);
 
   void ensure_parsability(bool retire_tlabs) override;
   void resize_all_tlabs() override;

src/hotspot/share/gc/parallel/psOldGen.cpp

@@ -217,9 +217,6 @@ bool PSOldGen::expand(size_t bytes) {
     success = expand_to_reserved();
   }
 
-  if (success && GCLocker::is_active_and_needs_gc()) {
-    log_debug(gc)("Garbage collection disabled, expanded heap instead");
-  }
   return success;
 }
 

src/hotspot/share/gc/parallel/psParallelCompact.cpp

@@ -993,10 +993,6 @@ bool PSParallelCompact::invoke_no_policy(bool clear_all_soft_refs) {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at a safepoint");
   assert(ref_processor() != nullptr, "Sanity");
 
-  if (GCLocker::check_active_before_gc()) {
-    return false;
-  }
-
   ParallelScavengeHeap* heap = ParallelScavengeHeap::heap();
 
   GCIdMark gc_id_mark;

src/hotspot/share/gc/parallel/psVMOperations.cpp

@@ -43,14 +43,10 @@ void VM_ParallelCollectForAllocation::doit() {
 
   GCCauseSetter gccs(heap, _gc_cause);
   _result = heap->satisfy_failed_allocation(_word_size, _is_tlab);
-
-  if (_result == nullptr && GCLocker::is_active_and_needs_gc()) {
-    set_gc_locked();
-  }
 }
 
 static bool is_cause_full(GCCause::Cause cause) {
-  return (cause != GCCause::_gc_locker) && (cause != GCCause::_wb_young_gc)
+  return (cause != GCCause::_wb_young_gc)
          DEBUG_ONLY(&& (cause != GCCause::_scavenge_alot));
 }
 
@@ -64,5 +60,5 @@ void VM_ParallelGCCollect::doit() {
   ParallelScavengeHeap* heap = ParallelScavengeHeap::heap();
 
   GCCauseSetter gccs(heap, _gc_cause);
-  heap->try_collect_at_safepoint(_full);
+  heap->collect_at_safepoint(_full);
 }

src/hotspot/share/gc/serial/defNewGeneration.cpp

@@ -367,18 +367,6 @@ bool DefNewGeneration::expand(size_t bytes) {
     SpaceMangler::mangle_region(mangle_region);
   }
 
-  // Do not attempt an expand-to-the reserve size.  The
-  // request should properly observe the maximum size of
-  // the generation so an expand-to-reserve should be
-  // unnecessary.  Also a second call to expand-to-reserve
-  // value potentially can cause an undue expansion.
-  // For example if the first expand fail for unknown reasons,
-  // but the second succeeds and expands the heap to its maximum
-  // value.
-  if (GCLocker::is_active()) {
-    log_debug(gc)("Garbage collection disabled, expanded heap instead");
-  }
-
   return success;
 }