Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8259242: Remove ProtectionDomainSet_lock #3362

Closed
wants to merge 4 commits into from
Closed

8259242: Remove ProtectionDomainSet_lock #3362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0695d6e
8259242: vmTestbase/vm/mlvm/mixed/stress/java/findDeadlock/TestDescri…
coleenp Apr 6, 2021
40c359b
Add acquire/release and update names.
coleenp Apr 7, 2021
06624ed
Make ProtectionDomainEntry delete list global and set a threshold for…
coleenp Apr 8, 2021
153eccd
Add overall comment and assert in count function.
coleenp Apr 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 45 additions & 38 deletions src/hotspot/share/classfile/dictionary.cpp
View file
Open in desktop
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;
coleenp marked this conversation as resolved.
Show resolved Hide resolved
#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);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested comment before this:
// Mark current for deletion, but in the meantime it can still be traversed

It is important that current is not unlinked so that in-progress traversals do not break. Though to be honest I'm unclear exactly what triggers the clearing of an entry this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the comment. The service thread could be trying to remove this entry and there's a small chance that some class loading will be looking up this entry with SystemDictionary::find_instance_klass.

// 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.

There's a test runtime/Dictionary/ProtectionDomainCache.java that does this. It creates a protection domain with a jar file loading with a URL class loader that's then removed. The defining class loader of the class loaded has that protection domain in its pd_set.

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
Open in desktop
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
Open in desktop
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.
Comment on lines +89 to +91
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand what you are trying to do here. This is basically a no-op handshake with every thread? Does all the threads remain in the handshake until the last one is processed or do they proceed through one at a time? The former seems better suited for a safepoint operation. The latter means this doesn't work as they may have moved on to a new removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the ServiceThread calls this code, so they won't be removed again. This is modeled after the ObjectMonitor code that does the same thing. All the threads have to hit a handshake at a safepoint polling place, which they cannot do while reading the list. After all the threads have hit the handshake, no thread can see the old entries on the list.
I thought that's what I said in the comment. What should I say to make this clearer?

Copy link
Member

Choose a reason for hiding this comment

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

See src/hotspot/share/runtime/synchronizer.cpp:

size_t ObjectSynchronizer::deflate_idle_monitors() {
<snip>
    if (current->is_Java_thread()) {
<snip>
      // A JavaThread needs to handshake in order to safely free the
      // ObjectMonitors that were deflated in this cycle.
      HandshakeForDeflation hfd_hc;
      Handshake::execute(&hfd_hc);

// 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);
}
Comment on lines +113 to +116
Copy link
Member

Choose a reason for hiding this comment

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

What is protecting this code to ensure the allocation can only happen once?

Copy link
Contributor Author

@coleenp coleenp Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This functions is only called from the service thread. It's not reentrant.


{
// 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
Open in desktop
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