8245925: G1 allocates EDEN region after CDS has executed GC

Reviewed-by: jiangli, minqi, tschatzl

Author: iklam

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -326,15 +326,14 @@ void G1HeapVerifier::verify_ready_for_archiving() {
   if (cl.has_holes()) {
     log_warning(gc, verify)("All free regions should be at the top end of the heap, but"
                             " we found holes. This is probably caused by (unmovable) humongous"
-                            " allocations, and may lead to fragmentation while"
+                            " allocations or active GCLocker, and may lead to fragmentation while"
                             " writing archive heap memory regions.");
   }
   if (cl.has_humongous()) {
     log_warning(gc, verify)("(Unmovable) humongous regions have been found and"
                             " may lead to fragmentation while"
                             " writing archive heap memory regions.");
   }
-  assert(!cl.has_unexpected_holes(), "all holes should have been caused by humongous regions");
 }
 
 class VerifyArchivePointerRegionClosure: public HeapRegionClosure {

src/hotspot/share/gc/shared/collectedHeap.cpp

@@ -240,6 +240,7 @@ void CollectedHeap::collect_as_vm_thread(GCCause::Cause cause) {
       do_full_collection(false);        // don't clear all soft refs
       break;
     }
+    case GCCause::_archive_time_gc:
     case GCCause::_metadata_GC_clear_soft_refs: {
       HandleMark hm;
       do_full_collection(true);         // do clear all soft refs

src/hotspot/share/memory/heapShared.cpp

@@ -28,6 +28,7 @@
 #include "classfile/symbolTable.hpp"
 #include "classfile/systemDictionaryShared.hpp"
 #include "classfile/vmSymbols.hpp"
+#include "gc/shared/gcLocker.hpp"
 #include "logging/log.hpp"
 #include "logging/logMessage.hpp"
 #include "logging/logStream.hpp"
@@ -186,6 +187,25 @@ void HeapShared::archive_klass_objects(Thread* THREAD) {
   }
 }
 
+void HeapShared::run_full_gc_in_vm_thread() {
+  if (is_heap_object_archiving_allowed()) {
+    // Avoid fragmentation while archiving heap objects.
+    // We do this inside a safepoint, so that no further allocation can happen after GC
+    // has finished.
+    if (GCLocker::is_active()) {
+      // Just checking for safety ...
+      // This should not happen during -Xshare:dump. If you see this, probably the Java core lib
+      // has been modified such that JNI code is executed in some clean up threads after
+      // we have finished class loading.
+      log_warning(cds)("GC locker is held, unable to start extra compacting GC. This may produce suboptimal results.");
+    } else {
+      log_info(cds)("Run GC ...");
+      Universe::heap()->collect_as_vm_thread(GCCause::_archive_time_gc);
+      log_info(cds)("Run GC done");
+    }
+  }
+}
+
 void HeapShared::archive_java_heap_objects(GrowableArray<MemRegion> *closed,
                                            GrowableArray<MemRegion> *open) {
   if (!is_heap_object_archiving_allowed()) {

src/hotspot/share/memory/heapShared.hpp

@@ -277,6 +277,8 @@ class HeapShared: AllStatic {
 #endif // INCLUDE_CDS_JAVA_HEAP
 
  public:
+  static void run_full_gc_in_vm_thread() NOT_CDS_JAVA_HEAP_RETURN;
+
   static bool is_heap_object_archiving_allowed() {
     CDS_JAVA_HEAP_ONLY(return (UseG1GC && UseCompressedOops && UseCompressedClassPointers);)
     NOT_CDS_JAVA_HEAP(return false;)

src/hotspot/share/memory/metaspaceShared.cpp

@@ -1638,6 +1638,7 @@ void VM_PopulateDumpSharedSpace::relocate_to_requested_base_address(CHeapBitMap*
 }
 
 void VM_PopulateDumpSharedSpace::doit() {
+  HeapShared::run_full_gc_in_vm_thread();
   CHeapBitMap ptrmap;
   MetaspaceShared::initialize_ptr_marker(&ptrmap);
 
@@ -1956,14 +1957,9 @@ void MetaspaceShared::preload_and_dump(TRAPS) {
     link_and_cleanup_shared_classes(CATCH);
     log_info(cds)("Rewriting and linking classes: done");
 
-    if (HeapShared::is_heap_object_archiving_allowed()) {
-      // Avoid fragmentation while archiving heap objects.
-      Universe::heap()->soft_ref_policy()->set_should_clear_all_soft_refs(true);
-      Universe::heap()->collect(GCCause::_archive_time_gc);
-      Universe::heap()->soft_ref_policy()->set_should_clear_all_soft_refs(false);
-    }
-
     VM_PopulateDumpSharedSpace op;
+    MutexLocker ml(THREAD, HeapShared::is_heap_object_archiving_allowed() ?
+                   Heap_lock : NULL);     // needed by HeapShared::run_gc()
     VMThread::execute(&op);
   }
 }

test/hotspot/jtreg/runtime/cds/appcds/javaldr/GCDuringDump.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 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
@@ -41,6 +41,7 @@ public class GCDuringDump {
     };
     public static String agentClasses[] = {
         GCDuringDumpTransformer.class.getName(),
+        GCDuringDumpTransformer.MyCleaner.class.getName(),
     };
 
     public static void main(String[] args) throws Throwable {
@@ -55,15 +56,18 @@ public static void main(String[] args) throws Throwable {
         String gcLog = Boolean.getBoolean("test.cds.verbose.gc") ?
             "-Xlog:gc*=info,gc+region=trace,gc+alloc+region=debug" : "-showversion";
 
-        for (int i=0; i<2; i++) {
+        for (int i=0; i<3; i++) {
             // i = 0 -- run without agent = no extra GCs
             // i = 1 -- run with agent = cause extra GCs
+            // i = 2 -- run with agent = cause extra GCs + use java.lang.ref.Cleaner
 
             String extraArg = (i == 0) ? "-showversion" : "-javaagent:" + agentJar;
             String extraOption = (i == 0) ? "-showversion" : "-XX:+AllowArchivingWithJavaAgent";
+            String extraOption2 = (i != 2) ? "-showversion" : "-Dtest.with.cleaner=true";
 
             TestCommon.testDump(appJar, TestCommon.list(Hello.class.getName()),
-                                "-XX:+UnlockDiagnosticVMOptions", extraOption,
+                                "-XX:+UnlockDiagnosticVMOptions", extraOption, extraOption2,
+                                "-Xlog:exceptions=trace",
                                 extraArg, "-Xmx32m", gcLog);
 
             TestCommon.run(

test/hotspot/jtreg/runtime/cds/appcds/javaldr/GCDuringDumpTransformer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 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
@@ -25,18 +25,40 @@
 import java.lang.instrument.ClassFileTransformer;
 import java.lang.instrument.Instrumentation;
 import java.lang.instrument.IllegalClassFormatException;
+import java.lang.ref.Cleaner;
 import java.security.ProtectionDomain;
 
 public class GCDuringDumpTransformer implements ClassFileTransformer {
+    static boolean TEST_WITH_CLEANER = Boolean.getBoolean("test.with.cleaner");
+    static Cleaner cleaner;
+    static Thread thread;
+    static Object garbage;
+
+    static {
+        if (TEST_WITH_CLEANER) {
+            cleaner = Cleaner.create();
+            garbage = new Object();
+            cleaner.register(garbage, new MyCleaner());
+            System.out.println("Registered cleaner");
+        }
+    }
+
     public byte[] transform(ClassLoader loader, String name, Class<?> classBeingRedefined,
                             ProtectionDomain pd, byte[] buffer) throws IllegalClassFormatException {
-        try {
-            makeGarbage();
-        } catch (Throwable t) {
-            t.printStackTrace();
+        if (TEST_WITH_CLEANER) {
+            if (name.equals("Hello")) {
+                garbage = null;
+                System.out.println("Unreferenced GCDuringDumpTransformer.garbage");
+            }
+        } else {
             try {
-                Thread.sleep(200); // let GC to have a chance to run
-            } catch (Throwable t2) {}
+                makeGarbage();
+            } catch (Throwable t) {
+                t.printStackTrace();
+                try {
+                    Thread.sleep(200); // let GC to have a chance to run
+                } catch (Throwable t2) {}
+            }
         }
 
         return null;
@@ -45,7 +67,7 @@ public byte[] transform(ClassLoader loader, String name, Class<?> classBeingRede
     private static Instrumentation savedInstrumentation;
 
     public static void premain(String agentArguments, Instrumentation instrumentation) {
-        System.out.println("ClassFileTransformer.premain() is called");
+        System.out.println("ClassFileTransformer.premain() is called: TEST_WITH_CLEANER = " + TEST_WITH_CLEANER);
         instrumentation.addTransformer(new GCDuringDumpTransformer(), /*canRetransform=*/true);
         savedInstrumentation = instrumentation;
     }
@@ -63,4 +85,15 @@ public static void makeGarbage() {
             Object[] a = new Object[10000];
         }
     }
+
+    static class MyCleaner implements Runnable {
+        static int count = 0;
+        int i = count++;
+        public void run() {
+            // Allocate something. This will cause G1 to allocate an EDEN region.
+            // See JDK-8245925
+            Object o = new Object();
+            System.out.println("cleaning " + i);
+        }
+    }
 }