8256641: CDS VM operations do not lock the heap

Reviewed-by: kbarrett, iklam

Author: Thomas Schatzl

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

@@ -472,6 +472,7 @@ bool G1HeapVerifier::should_verify(G1VerifyType type) {
 
 void G1HeapVerifier::verify(VerifyOption vo) {
   assert_at_safepoint_on_vm_thread();
+  assert(Heap_lock->is_locked(), "heap must be locked");
 
   log_debug(gc, verify)("Roots");
   VerifyRootsClosure rootsCl(vo);

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

@@ -46,6 +46,20 @@
 #include "gc/g1/g1Policy.hpp"
 #endif // INCLUDE_G1GC
 
+bool VM_GC_Sync_Operation::doit_prologue() {
+  Heap_lock->lock();
+  return true;
+}
+
+void VM_GC_Sync_Operation::doit_epilogue() {
+  Heap_lock->unlock();
+}
+
+void VM_Verify::doit() {
+  Universe::heap()->prepare_for_verify();
+  Universe::verify();
+}
+
 VM_GC_Operation::~VM_GC_Operation() {
   CollectedHeap* ch = Universe::heap();
   ch->soft_ref_policy()->set_all_soft_refs_clear(false);
@@ -94,8 +108,7 @@ bool VM_GC_Operation::doit_prologue() {
               proper_unit_for_byte_size(NewSize)));
   }
 
-  // If the GC count has changed someone beat us to the collection
-  Heap_lock->lock();
+  VM_GC_Sync_Operation::doit_prologue();
 
   // Check invocations
   if (skip_operation()) {
@@ -116,7 +129,7 @@ void VM_GC_Operation::doit_epilogue() {
   if (Universe::has_reference_pending_list()) {
     Heap_lock->notify_all();
   }
-  Heap_lock->unlock();
+  VM_GC_Sync_Operation::doit_epilogue();
 }
 
 bool VM_GC_HeapInspection::skip_operation() const {

src/hotspot/share/gc/shared/gcVMOperations.hpp

@@ -39,17 +39,27 @@
 // a set of operations (VM_Operation) related to GC.
 //
 //  VM_Operation
+//    VM_GC_Sync_Operation
 //      VM_GC_Operation
-//          VM_GC_HeapInspection
-//          VM_GenCollectFull
-//          VM_GenCollectFullConcurrent
-//          VM_ParallelGCSystemGC
-//          VM_CollectForAllocation
-//              VM_GenCollectForAllocation
-//              VM_ParallelGCFailedAllocation
+//        VM_GC_HeapInspection
+//        VM_PopulateDynamicDumpSharedSpace
+//        VM_GenCollectFull
+//        VM_GenCollectFullConcurrent
+//        VM_ParallelGCSystemGC
+//        VM_CollectForAllocation
+//          VM_GenCollectForAllocation
+//          VM_ParallelGCFailedAllocation
+//      VM_Verify
+//      VM_PopulateDumpSharedSpace
+//
+//  VM_GC_Sync_Operation
+//   - implements only synchronization with other VM operations of the
+//     same kind using the Heap_lock, not actually doing a GC.
+//
 //  VM_GC_Operation
-//   - implements methods common to all classes in the hierarchy:
-//     prevents multiple gc requests and manages lock on heap;
+//   - implements methods common to all operations that perform garbage collections,
+//     checking that the VM is in a state to do GC and preventing multiple GC
+//     requests.
 //
 //  VM_GC_HeapInspection
 //   - prints class histogram on SIGBREAK if PrintClassHistogram
@@ -68,11 +78,37 @@
 //   - these operations preform full collection of heaps of
 //     different kind
 //
+//  VM_Verify
+//   - verifies the heap
+//
+//  VM_PopulateDynamicDumpSharedSpace
+//   - populates the CDS archive area with the information from the archive file.
+//
+//  VM_PopulateDumpSharedSpace
+//   - creates the CDS archive
+//
+
+class VM_GC_Sync_Operation : public VM_Operation {
+public:
+
+  VM_GC_Sync_Operation() : VM_Operation() { }
+
+  // Acquires the Heap_lock.
+  virtual bool doit_prologue();
+  // Releases the Heap_lock.
+  virtual void doit_epilogue();
+};
+
+class VM_Verify : public VM_GC_Sync_Operation {
+ public:
+  VMOp_Type type() const { return VMOp_Verify; }
+  void doit();
+};
 
-class VM_GC_Operation: public VM_Operation {
+class VM_GC_Operation: public VM_GC_Sync_Operation {
  protected:
-  uint           _gc_count_before;         // gc count before acquiring PLL
-  uint           _full_gc_count_before;    // full gc count before acquiring PLL
+  uint           _gc_count_before;         // gc count before acquiring the Heap_lock
+  uint           _full_gc_count_before;    // full gc count before acquiring the Heap_lock
   bool           _full;                    // whether a "full" collection
   bool           _prologue_succeeded;      // whether doit_prologue succeeded
   GCCause::Cause _gc_cause;                // the putative cause for this gc op
@@ -84,7 +120,7 @@ class VM_GC_Operation: public VM_Operation {
   VM_GC_Operation(uint gc_count_before,
                   GCCause::Cause _cause,
                   uint full_gc_count_before = 0,
-                  bool full = false) {
+                  bool full = false) : VM_GC_Sync_Operation() {
     _full = full;
     _prologue_succeeded = false;
     _gc_count_before    = gc_count_before;
@@ -106,9 +142,10 @@ class VM_GC_Operation: public VM_Operation {
   }
   ~VM_GC_Operation();
 
-  // Acquire the reference synchronization lock
+  // Acquire the Heap_lock and determine if this VM operation should be executed
+  // (i.e. not skipped). Return this result, and also store it in _prologue_succeeded.
   virtual bool doit_prologue();
-  // Do notifyAll (if needed) and release held lock
+  // Notify the Heap_lock if needed and release it.
   virtual void doit_epilogue();
 
   virtual bool allow_nested_vm_operations() const  { return true; }

src/hotspot/share/gc/z/zDriver.cpp

@@ -24,6 +24,7 @@
 #include "precompiled.hpp"
 #include "gc/shared/gcId.hpp"
 #include "gc/shared/gcLocker.hpp"
+#include "gc/shared/gcVMOperations.hpp"
 #include "gc/shared/isGCActiveMark.hpp"
 #include "gc/z/zBreakpoint.hpp"
 #include "gc/z/zCollectedHeap.hpp"

src/hotspot/share/memory/dynamicArchive.cpp

@@ -27,6 +27,7 @@
 #include "classfile/classLoaderData.inline.hpp"
 #include "classfile/symbolTable.hpp"
 #include "classfile/systemDictionaryShared.hpp"
+#include "gc/shared/gcVMOperations.hpp"
 #include "logging/log.hpp"
 #include "memory/archiveBuilder.hpp"
 #include "memory/archiveUtils.inline.hpp"
@@ -541,10 +542,10 @@ void DynamicArchiveBuilder::write_archive(char* serialized_data) {
   log_info(cds, dynamic)("%d klasses; %d symbols", num_klasses, num_symbols);
 }
 
-class VM_PopulateDynamicDumpSharedSpace: public VM_Operation {
+class VM_PopulateDynamicDumpSharedSpace: public VM_GC_Sync_Operation {
   DynamicArchiveBuilder* _builder;
 public:
-  VM_PopulateDynamicDumpSharedSpace(DynamicArchiveBuilder* builder) : _builder(builder) {}
+  VM_PopulateDynamicDumpSharedSpace(DynamicArchiveBuilder* builder) : VM_GC_Sync_Operation(), _builder(builder) {}
   VMOp_Type type() const { return VMOp_PopulateDumpSharedSpace; }
   void doit() {
     ResourceMark rm;

src/hotspot/share/memory/heapShared.cpp

@@ -33,6 +33,7 @@
 #include "classfile/systemDictionaryShared.hpp"
 #include "classfile/vmSymbols.hpp"
 #include "gc/shared/gcLocker.hpp"
+#include "gc/shared/gcVMOperations.hpp"
 #include "logging/log.hpp"
 #include "logging/logMessage.hpp"
 #include "logging/logStream.hpp"
@@ -655,8 +656,10 @@ static void verify_the_heap(Klass* k, const char* which) {
     ResourceMark rm;
     log_info(cds, heap)("Verify heap %s initializing static field(s) in %s",
                         which, k->external_name());
+
     VM_Verify verify_op;
     VMThread::execute(&verify_op);
+
     if (!FLAG_IS_DEFAULT(VerifyArchivedFields)) {
       // If VerifyArchivedFields has a non-default value (e.g., specified on the command-line), do
       // more expensive checks.

src/hotspot/share/memory/metaspaceShared.cpp

@@ -37,6 +37,7 @@
 #include "classfile/systemDictionaryShared.hpp"
 #include "classfile/vmSymbols.hpp"
 #include "code/codeCache.hpp"
+#include "gc/shared/gcVMOperations.hpp"
 #include "interpreter/abstractInterpreter.hpp"
 #include "interpreter/bytecodeStream.hpp"
 #include "interpreter/bytecodes.hpp"
@@ -577,7 +578,7 @@ void MetaspaceShared::rewrite_nofast_bytecodes_and_calculate_fingerprints(Thread
   }
 }
 
-class VM_PopulateDumpSharedSpace: public VM_Operation {
+class VM_PopulateDumpSharedSpace : public VM_GC_Operation {
 private:
   GrowableArray<MemRegion> *_closed_archive_heap_regions;
   GrowableArray<MemRegion> *_open_archive_heap_regions;
@@ -602,6 +603,12 @@ class VM_PopulateDumpSharedSpace: public VM_Operation {
 
 public:
 
+  VM_PopulateDumpSharedSpace() : VM_GC_Operation(0, /* total collections, ignored */
+                                                 GCCause::_archive_time_gc)
+  { }
+
+  bool skip_operation() const { return false; }
+
   VMOp_Type type() const { return VMOp_PopulateDumpSharedSpace; }
   void doit();   // outline because gdb sucks
   bool allow_nested_vm_operations() const { return true; }
@@ -1085,8 +1092,6 @@ void MetaspaceShared::preload_and_dump(TRAPS) {
 #endif
 
     VM_PopulateDumpSharedSpace op;
-    MutexLocker ml(THREAD, HeapShared::is_heap_object_archiving_allowed() ?
-                   Heap_lock : NULL);     // needed by HeapShared::run_gc()
     VMThread::execute(&op);
   }
 }

src/hotspot/share/runtime/thread.cpp

@@ -38,6 +38,7 @@
 #include "gc/shared/barrierSet.hpp"
 #include "gc/shared/gcId.hpp"
 #include "gc/shared/gcLocker.inline.hpp"
+#include "gc/shared/gcVMOperations.hpp"
 #include "gc/shared/oopStorage.hpp"
 #include "gc/shared/oopStorageSet.hpp"
 #include "gc/shared/workgroup.hpp"

src/hotspot/share/runtime/vmOperations.cpp

@@ -156,11 +156,6 @@ void VM_ZombieAll::doit() {
 
 #endif // !PRODUCT
 
-void VM_Verify::doit() {
-  Universe::heap()->prepare_for_verify();
-  Universe::verify();
-}
-
 bool VM_PrintThreads::doit_prologue() {
   // Get Heap_lock if concurrent locks will be dumped
   if (_print_concurrent_locks) {

src/hotspot/share/runtime/vmOperations.hpp

@@ -277,13 +277,6 @@ class VM_ZombieAll: public VM_Operation {
 };
 #endif // PRODUCT
 
-class VM_Verify: public VM_Operation {
- public:
-  VMOp_Type type() const { return VMOp_Verify; }
-  void doit();
-};
-
-
 class VM_PrintThreads: public VM_Operation {
  private:
   outputStream* _out;