8233822: VM_G1CollectForAllocation should always check for upgrade to…

… full

Move upgrade check into do_collection_pause_at_safepoint.

Reviewed-by: tschatzl, sangheki

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

@@ -2987,6 +2987,19 @@ bool G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_
     return false;
   }
 
+  do_collection_pause_at_safepoint_helper(target_pause_time_ms);
+  if (should_upgrade_to_full_gc(gc_cause())) {
+    log_info(gc, ergo)("Attempting maximally compacting collection");
+    bool result = do_full_collection(false /* explicit gc */,
+                                     true /* clear_all_soft_refs */);
+    // do_full_collection only fails if blocked by GC locker, but
+    // we've already checked for that above.
+    assert(result, "invariant");
+  }
+  return true;
+}
+
+void G1CollectedHeap::do_collection_pause_at_safepoint_helper(double target_pause_time_ms) {
   GCIdMark gc_id_mark;
 
   SvcGCMarker sgcm(SvcGCMarker::MINOR);
@@ -3174,8 +3187,6 @@ bool G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_
     // itself is released in SuspendibleThreadSet::desynchronize().
     do_concurrent_mark();
   }
-
-  return true;
 }
 
 void G1CollectedHeap::remove_self_forwarding_pointers(G1RedirtyCardsQueueSet* rdcqs) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -757,11 +757,18 @@ class G1CollectedHeap : public CollectedHeap {
 
   void wait_for_root_region_scanning();
 
-  // The guts of the incremental collection pause, executed by the vm
-  // thread. It returns false if it is unable to do the collection due
-  // to the GC locker being active, true otherwise
+  // Perform an incremental collection at a safepoint, possibly
+  // followed by a by-policy upgrade to a full collection.  Returns
+  // false if unable to do the collection due to the GC locker being
+  // active, true otherwise.
+  // precondition: at safepoint on VM thread
+  // precondition: !is_gc_active()
   bool do_collection_pause_at_safepoint(double target_pause_time_ms);
 
+  // Helper for do_collection_pause_at_safepoint, containing the guts
+  // of the incremental collection pause, executed by the vm thread.
+  void do_collection_pause_at_safepoint_helper(double target_pause_time_ms);
+
   G1HeapVerifier::G1VerifyType young_collection_verify_type() const;
   void verify_before_young_collection(G1HeapVerifier::G1VerifyType type);
   void verify_after_young_collection(G1HeapVerifier::G1VerifyType type);

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

@@ -82,20 +82,14 @@ void VM_G1TryInitiateConcMark::doit() {
     // there is already a concurrent marking cycle in progress.  Set flag
     // to notify the caller and return immediately.
     _cycle_already_in_progress = true;
-  } else if (!g1h->do_collection_pause_at_safepoint(_target_pause_time_ms)) {
+  } else if (g1h->do_collection_pause_at_safepoint(_target_pause_time_ms)) {
+    _gc_succeeded = true;
+  } else {
     // Failure to perform the collection at all occurs because GCLocker is
     // active, and we have the bad luck to be the collection request that
     // makes a later _gc_locker collection needed.  (Else we would have hit
     // the GCLocker check in the prologue.)
     _transient_failure = true;
-  } else if (g1h->should_upgrade_to_full_gc(_gc_cause)) {
-    // GC ran, but we're still in trouble and need a full GC.
-    log_info(gc, ergo)("Attempting maximally compacting collection");
-    _gc_succeeded = g1h->do_full_collection(false, /* explicit gc */
-                                            true /* clear_all_soft_refs */);
-    guarantee(_gc_succeeded, "Elevated collections during the safepoint must always succeed");
-  } else {
-    _gc_succeeded = true;
   }
 }
 
@@ -132,20 +126,10 @@ void VM_G1CollectForAllocation::doit() {
   // Try a partial collection of some kind.
   _gc_succeeded = g1h->do_collection_pause_at_safepoint(_target_pause_time_ms);
 
-  if (_gc_succeeded) {
-    if (_word_size > 0) {
-      // An allocation had been requested. Do it, eventually trying a stronger
-      // kind of GC.
-      _result = g1h->satisfy_failed_allocation(_word_size, &_gc_succeeded);
-    } else if (g1h->should_upgrade_to_full_gc(_gc_cause)) {
-      // There has been a request to perform a GC to free some space. We have no
-      // information on how much memory has been asked for. In case there are
-      // absolutely no regions left to allocate into, do a maximally compacting full GC.
-      log_info(gc, ergo)("Attempting maximally compacting collection");
-      _gc_succeeded = g1h->do_full_collection(false, /* explicit gc */
-                                              true   /* clear_all_soft_refs */);
-    }
-    guarantee(_gc_succeeded, "Elevated collections during the safepoint must always succeed.");
+  if (_gc_succeeded && (_word_size > 0)) {
+    // An allocation had been requested. Do it, eventually trying a stronger
+    // kind of GC.
+    _result = g1h->satisfy_failed_allocation(_word_size, &_gc_succeeded);
   }
 }
 

src/hotspot/share/gc/g1/g1VMOperations.hpp

@@ -29,10 +29,6 @@
 #include "gc/shared/gcVMOperations.hpp"
 
 // VM_operations for the G1 collector.
-// VM_GC_Operation:
-//   - VM_G1Concurrent
-//   - VM_G1CollectForAllocation
-//   - VM_G1CollectFull
 
 class VM_G1CollectFull : public VM_GC_Operation {
   bool _gc_succeeded;