Skip to content

Commit

Permalink
8259242: Remove ProtectionDomainSet_lock
Browse files Browse the repository at this point in the history 
Reviewed-by: dholmes, pchilanomate
  • Loading branch information
@coleenp
coleenp committed Apr 9, 2021
1 parent 9bb1863 commit 06e6b1f
Show file tree
Hide file tree
Showing 10 changed files with 129 additions and 82 deletions.
83 changes: 45 additions & 38 deletions src/hotspot/share/classfile/dictionary.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "runtime/javaCalls.hpp"
#include "runtime/mutexLocker.hpp"
#include "runtime/safepointVerifiers.hpp"
#include "utilities/growableArray.hpp"
#include "utilities/hashtable.inline.hpp"

// Optimization: if any dictionary needs resizing, we set this flag,
Expand Down Expand Up @@ -78,20 +79,19 @@ Dictionary::~Dictionary() {

DictionaryEntry* Dictionary::new_entry(unsigned int hash, InstanceKlass* klass) {
DictionaryEntry* entry = (DictionaryEntry*)Hashtable<InstanceKlass*, mtClass>::new_entry(hash, klass);
entry->set_pd_set(NULL);
entry->release_set_pd_set(NULL);
assert(klass->is_instance_klass(), "Must be");
return entry;
}


void Dictionary::free_entry(DictionaryEntry* entry) {
// avoid recursion when deleting linked list
// pd_set is accessed during a safepoint.
// This doesn't require a lock because nothing is reading this
// entry anymore. The ClassLoader is dead.
while (entry->pd_set() != NULL) {
ProtectionDomainEntry* to_delete = entry->pd_set();
entry->set_pd_set(to_delete->next());
while (entry->pd_set_acquire() != NULL) {
ProtectionDomainEntry* to_delete = entry->pd_set_acquire();
entry->release_set_pd_set(to_delete->next_acquire());
delete to_delete;
}
BasicHashtable<mtClass>::free_entry(entry);
Expand Down Expand Up @@ -141,15 +141,26 @@ bool DictionaryEntry::is_valid_protection_domain(Handle protection_domain) {
: contains_protection_domain(protection_domain());
}

// Reading the pd_set on each DictionaryEntry is lock free and cannot safepoint.
// Adding and deleting entries is under the SystemDictionary_lock
// Deleting unloaded entries on ClassLoaderData for dictionaries that are not unloaded
// is a three step process:
// moving the entries to a separate list, handshake to wait for
// readers to complete (see NSV here), and then actually deleting the entries.
// Deleting entries is done by the ServiceThread when triggered by class unloading.

bool DictionaryEntry::contains_protection_domain(oop protection_domain) const {
assert(Thread::current()->is_Java_thread() || SafepointSynchronize::is_at_safepoint(),
"can only be called by a JavaThread or at safepoint");
// This cannot safepoint while reading the protection domain set.
NoSafepointVerifier nsv;
#ifdef ASSERT
if (protection_domain == instance_klass()->protection_domain()) {
MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
// Ensure this doesn't show up in the pd_set (invariant)
bool in_pd_set = false;
for (ProtectionDomainEntry* current = pd_set();
for (ProtectionDomainEntry* current = pd_set_acquire();
current != NULL;
current = current->next()) {
current = current->next_acquire()) {
if (current->object_no_keepalive() == protection_domain) {
in_pd_set = true;
break;
Expand All @@ -167,30 +178,23 @@ bool DictionaryEntry::contains_protection_domain(oop protection_domain) const {
return true;
}

// Lock the pd_set list. This lock cannot safepoint since the caller holds
// a Dictionary entry, which can be moved if the Dictionary is resized.
MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
for (ProtectionDomainEntry* current = pd_set();
for (ProtectionDomainEntry* current = pd_set_acquire();
current != NULL;
current = current->next()) {
current = current->next_acquire()) {
if (current->object_no_keepalive() == protection_domain) {
return true;
}
}
return false;
}


void DictionaryEntry::add_protection_domain(Dictionary* dict, Handle protection_domain) {
assert_locked_or_safepoint(SystemDictionary_lock);
if (!contains_protection_domain(protection_domain())) {
ProtectionDomainCacheEntry* entry = SystemDictionary::cache_get(protection_domain);
// The pd_set in the dictionary entry is protected by a low level lock.
// With concurrent PD table cleanup, these links could be broken.
MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
ProtectionDomainEntry* new_head =
new ProtectionDomainEntry(entry, pd_set());
set_pd_set(new_head);
// Additions and deletions hold the SystemDictionary_lock, readers are lock-free
ProtectionDomainEntry* new_head = new ProtectionDomainEntry(entry, _pd_set);
release_set_pd_set(new_head);
}
LogTarget(Trace, protectiondomain) lt;
if (lt.is_enabled()) {
Expand Down Expand Up @@ -420,8 +424,9 @@ void Dictionary::validate_protection_domain(unsigned int name_hash,

// During class loading we may have cached a protection domain that has
// since been unreferenced, so this entry should be cleared.
void Dictionary::clean_cached_protection_domains() {
assert_locked_or_safepoint(SystemDictionary_lock);
void Dictionary::clean_cached_protection_domains(GrowableArray<ProtectionDomainEntry*>* delete_list) {
assert(Thread::current()->is_Java_thread(), "only called by JavaThread");
assert_lock_strong(SystemDictionary_lock);
assert(!loader_data()->has_class_mirror_holder(), "cld should have a ClassLoader holder not a Class holder");

if (loader_data()->is_the_null_class_loader_data()) {
Expand All @@ -435,8 +440,7 @@ void Dictionary::clean_cached_protection_domains() {
probe = probe->next()) {
Klass* e = probe->instance_klass();

MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
ProtectionDomainEntry* current = probe->pd_set();
ProtectionDomainEntry* current = probe->pd_set_acquire();
ProtectionDomainEntry* prev = NULL;
while (current != NULL) {
if (current->object_no_keepalive() == NULL) {
Expand All @@ -450,18 +454,19 @@ void Dictionary::clean_cached_protection_domains() {
ls.print(" loading: "); probe->instance_klass()->print_value_on(&ls);
ls.cr();
}
if (probe->pd_set() == current) {
probe->set_pd_set(current->next());
if (probe->pd_set_acquire() == current) {
probe->release_set_pd_set(current->next_acquire());
} else {
assert(prev != NULL, "should be set by alive entry");
prev->set_next(current->next());
prev->release_set_next(current->next_acquire());
}
ProtectionDomainEntry* to_delete = current;
current = current->next();
delete to_delete;
// Mark current for deletion but in the meantime it can still be
// traversed.
delete_list->push(current);
current = current->next_acquire();
} else {
prev = current;
current = current->next();
current = current->next_acquire();
}
}
}
Expand Down Expand Up @@ -552,20 +557,20 @@ void SymbolPropertyTable::free_entry(SymbolPropertyEntry* entry) {
}

void DictionaryEntry::verify_protection_domain_set() {
MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
for (ProtectionDomainEntry* current = pd_set(); // accessed at a safepoint
assert(SafepointSynchronize::is_at_safepoint(), "must only be called as safepoint");
for (ProtectionDomainEntry* current = pd_set_acquire(); // accessed at a safepoint
current != NULL;
current = current->_next) {
guarantee(oopDesc::is_oop_or_null(current->_pd_cache->object_no_keepalive()), "Invalid oop");
current = current->next_acquire()) {
guarantee(oopDesc::is_oop_or_null(current->object_no_keepalive()), "Invalid oop");
}
}

void DictionaryEntry::print_count(outputStream *st) {
MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
assert_locked_or_safepoint(SystemDictionary_lock);
int count = 0;
for (ProtectionDomainEntry* current = pd_set(); // accessed inside SD lock
for (ProtectionDomainEntry* current = pd_set_acquire();
current != NULL;
current = current->_next) {
current = current->next_acquire()) {
count++;
}
st->print_cr("pd set count = #%d", count);
Expand Down Expand Up @@ -596,6 +601,8 @@ void Dictionary::print_on(outputStream* st) const {
// redundant and obvious.
st->print(", ");
cld->print_value_on(st);
st->print(", ");
probe->print_count(st);
}
st->cr();
}
Expand Down
18 changes: 6 additions & 12 deletions src/hotspot/share/classfile/dictionary.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

class DictionaryEntry;
class ProtectionDomainEntry;
template <typename T> class GrowableArray;

//~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// The data structure for the class loader data dictionaries.
Expand Down Expand Up @@ -67,7 +68,7 @@ class Dictionary : public Hashtable<InstanceKlass*, mtClass> {
void all_entries_do(KlassClosure* closure);
void classes_do(MetaspaceClosure* it);

void clean_cached_protection_domains();
void clean_cached_protection_domains(GrowableArray<ProtectionDomainEntry*>* delete_list);

// Protection domains
InstanceKlass* find(unsigned int hash, Symbol* name, Handle protection_domain);
Expand Down Expand Up @@ -111,18 +112,11 @@ class DictionaryEntry : public HashtableEntry<InstanceKlass*, mtClass> {
// Contains the set of approved protection domains that can access
// this dictionary entry.
//
// This protection domain set is a set of tuples:
//
// (InstanceKlass C, initiating class loader ICL, Protection Domain PD)
//
// [Note that C.protection_domain(), which is stored in the java.lang.Class
// mirror of C, is NOT the same as PD]
//
// If such an entry (C, ICL, PD) exists in the table, it means that
// it is okay for a class Foo to reference C, where
//
// Foo.protection_domain() == PD, and
// Foo's defining class loader == ICL
// If an entry for PD exists in the list, it means that
// it is okay for a caller class to reference the class in this dictionary entry.
//
// The usage of the PD set can be seen in SystemDictionary::validate_protection_domain()
// It is essentially a cache to avoid repeated Java up-calls to
Expand All @@ -147,8 +141,8 @@ class DictionaryEntry : public HashtableEntry<InstanceKlass*, mtClass> {
return (DictionaryEntry**)HashtableEntry<InstanceKlass*, mtClass>::next_addr();
}

ProtectionDomainEntry* pd_set() const { return _pd_set; }
void set_pd_set(ProtectionDomainEntry* new_head) { _pd_set = new_head; }
ProtectionDomainEntry* pd_set_acquire() const { return Atomic::load_acquire(&_pd_set); }
void release_set_pd_set(ProtectionDomainEntry* entry) { Atomic::release_store(&_pd_set, entry); }

// Tells whether the initiating class' protection domain can access the klass in this entry
inline bool is_valid_protection_domain(Handle protection_domain);
Expand Down
56 changes: 50 additions & 6 deletions src/hotspot/share/classfile/protectionDomainCache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
#include "memory/universe.hpp"
#include "oops/oop.inline.hpp"
#include "oops/weakHandle.inline.hpp"
#include "runtime/atomic.hpp"
#include "utilities/growableArray.hpp"
#include "utilities/hashtable.inline.hpp"

unsigned int ProtectionDomainCacheTable::compute_hash(Handle protection_domain) {
Expand All @@ -58,29 +60,75 @@ void ProtectionDomainCacheTable::trigger_cleanup() {
}

class CleanProtectionDomainEntries : public CLDClosure {
GrowableArray<ProtectionDomainEntry*>* _delete_list;
public:
CleanProtectionDomainEntries(GrowableArray<ProtectionDomainEntry*>* delete_list) :
_delete_list(delete_list) {}

void do_cld(ClassLoaderData* data) {
Dictionary* dictionary = data->dictionary();
if (dictionary != NULL) {
dictionary->clean_cached_protection_domains();
dictionary->clean_cached_protection_domains(_delete_list);
}
}
};

static GrowableArray<ProtectionDomainEntry*>* _delete_list = NULL;

class HandshakeForPD : public HandshakeClosure {
public:
HandshakeForPD() : HandshakeClosure("HandshakeForPD") {}

void do_thread(Thread* thread) {
log_trace(protectiondomain)("HandshakeForPD::do_thread: thread="
INTPTR_FORMAT, p2i(thread));
}
};

static void purge_deleted_entries() {
// If there are any deleted entries, Handshake-all then they'll be
// safe to remove since traversing the pd_set list does not stop for
// safepoints and only JavaThreads will read the pd_set.
// This is actually quite rare because the protection domain is generally associated
// with the caller class and class loader, which if still alive will keep this
// protection domain entry alive.
if (_delete_list->length() >= 10) {
HandshakeForPD hs_pd;
Handshake::execute(&hs_pd);

for (int i = _delete_list->length() - 1; i >= 0; i--) {
ProtectionDomainEntry* entry = _delete_list->at(i);
_delete_list->remove_at(i);
delete entry;
}
assert(_delete_list->length() == 0, "should be cleared");
}
}

void ProtectionDomainCacheTable::unlink() {
// The dictionary entries _pd_set field should be null also, so nothing to do.
assert(java_lang_System::allow_security_manager(), "should not be called otherwise");

// Create a list for holding deleted entries
if (_delete_list == NULL) {
_delete_list = new (ResourceObj::C_HEAP, mtClass)
GrowableArray<ProtectionDomainEntry*>(20, mtClass);
}

{
// First clean cached pd lists in loaded CLDs
// It's unlikely, but some loaded classes in a dictionary might
// point to a protection_domain that has been unloaded.
// The dictionary pd_set points at entries in the ProtectionDomainCacheTable.
MutexLocker ml(ClassLoaderDataGraph_lock);
MutexLocker mldict(SystemDictionary_lock); // need both.
CleanProtectionDomainEntries clean;
CleanProtectionDomainEntries clean(_delete_list);
ClassLoaderDataGraph::loaded_cld_do(&clean);
}

// Purge any deleted entries outside of the SystemDictionary_lock.
purge_deleted_entries();

MutexLocker ml(SystemDictionary_lock);
int oops_removed = 0;
for (int i = 0; i < table_size(); ++i) {
Expand Down Expand Up @@ -129,10 +177,6 @@ oop ProtectionDomainCacheEntry::object() {
return literal().resolve();
}

oop ProtectionDomainEntry::object() {
return _pd_cache->object();
}

// The object_no_keepalive() call peeks at the phantomly reachable oop without
// keeping it alive. This is okay to do in the VM thread state if it is not
// leaked out to become strongly reachable.
Expand Down
23 changes: 9 additions & 14 deletions src/hotspot/share/classfile/protectionDomainCache.hpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2017, 2021, 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
Expand Down Expand Up @@ -27,7 +27,7 @@

#include "oops/oop.hpp"
#include "oops/weakHandle.hpp"
#include "memory/iterator.hpp"
#include "runtime/atomic.hpp"
#include "utilities/hashtable.hpp"

// This class caches the approved protection domains that can access loaded classes.
Expand Down Expand Up @@ -62,8 +62,6 @@ class ProtectionDomainCacheEntry : public HashtableEntry<WeakHandle, mtClass> {
// The amount of different protection domains used is typically magnitudes smaller
// than the number of system dictionary entries (loaded classes).
class ProtectionDomainCacheTable : public Hashtable<WeakHandle, mtClass> {
friend class VMStructs;
private:
ProtectionDomainCacheEntry* bucket(int i) const {
return (ProtectionDomainCacheEntry*) Hashtable<WeakHandle, mtClass>::bucket(i);
}
Expand Down Expand Up @@ -104,20 +102,17 @@ class ProtectionDomainCacheTable : public Hashtable<WeakHandle, mtClass> {
};


// This describes the linked list protection domain for each DictionaryEntry in pd_set.
class ProtectionDomainEntry :public CHeapObj<mtClass> {
friend class VMStructs;
public:
ProtectionDomainEntry* _next;
ProtectionDomainCacheEntry* _pd_cache;
ProtectionDomainEntry* volatile _next;
public:

ProtectionDomainEntry(ProtectionDomainCacheEntry* pd_cache, ProtectionDomainEntry* next) {
_pd_cache = pd_cache;
_next = next;
}
ProtectionDomainEntry(ProtectionDomainCacheEntry* pd_cache,
ProtectionDomainEntry* head) : _pd_cache(pd_cache), _next(head) {}

ProtectionDomainEntry* next() { return _next; }
void set_next(ProtectionDomainEntry* entry) { _next = entry; }
oop object();
ProtectionDomainEntry* next_acquire() { return Atomic::load_acquire(&_next); }
void release_set_next(ProtectionDomainEntry* entry) { Atomic::release_store(&_next, entry); }
oop object_no_keepalive();
};
#endif // SHARE_CLASSFILE_PROTECTIONDOMAINCACHE_HPP
Loading

1 comment on commit 06e6b1f

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.