Skip to content
This repository has been archived by the owner. It is now read-only.
Permalink
Browse files
8256641: CDS VM operations do not lock the heap
Reviewed-by: kbarrett, iklam
  • Loading branch information
Thomas Schatzl committed Dec 11, 2020
1 parent 58dca92 commit bacf22b907731172e36285ddacfc702136615aba
@@ -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);
@@ -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 {
@@ -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; }
@@ -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"
@@ -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;
@@ -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.
@@ -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);
}
}
@@ -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"
@@ -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) {
@@ -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;

1 comment on commit bacf22b

@openjdk-notifier

This comment has been minimized.

Copy link

@openjdk-notifier openjdk-notifier bot commented on bacf22b Dec 11, 2020

Please sign in to comment.