From 586167cff5aaead0949c509f48bc5080834cc362 Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Tue, 30 Sep 2025 08:49:08 +0000 Subject: [PATCH] 8363932: G1: Better distribute KlassCleaningTask Reviewed-by: ayang, coleenp --- .../share/classfile/classLoaderData.hpp | 2 +- .../share/classfile/classLoaderDataGraph.cpp | 63 ++++--------------- .../share/classfile/classLoaderDataGraph.hpp | 18 +++--- .../share/gc/shared/parallelCleaning.cpp | 52 ++++++--------- .../share/gc/shared/parallelCleaning.hpp | 8 +-- src/hotspot/share/oops/klass.cpp | 22 +++---- src/hotspot/share/oops/klass.hpp | 6 +- 7 files changed, 58 insertions(+), 113 deletions(-) diff --git a/src/hotspot/share/classfile/classLoaderData.hpp b/src/hotspot/share/classfile/classLoaderData.hpp index da49a9326e3..64fcfb7519f 100644 --- a/src/hotspot/share/classfile/classLoaderData.hpp +++ b/src/hotspot/share/classfile/classLoaderData.hpp @@ -97,7 +97,7 @@ class ClassLoaderData : public CHeapObj { }; friend class ClassLoaderDataGraph; - friend class ClassLoaderDataGraphKlassIteratorAtomic; + friend class ClassLoaderDataGraphIteratorAtomic; friend class Klass; friend class MetaDataFactory; friend class Method; diff --git a/src/hotspot/share/classfile/classLoaderDataGraph.cpp b/src/hotspot/share/classfile/classLoaderDataGraph.cpp index 4d3d6a951c5..61404fdf9db 100644 --- a/src/hotspot/share/classfile/classLoaderDataGraph.cpp +++ b/src/hotspot/share/classfile/classLoaderDataGraph.cpp @@ -489,62 +489,25 @@ void ClassLoaderDataGraph::purge(bool at_safepoint) { } } -ClassLoaderDataGraphKlassIteratorAtomic::ClassLoaderDataGraphKlassIteratorAtomic() - : _next_klass(nullptr) { +ClassLoaderDataGraphIteratorAtomic::ClassLoaderDataGraphIteratorAtomic() + : _cld(nullptr) { assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint!"); - ClassLoaderData* cld = ClassLoaderDataGraph::_head; - Klass* klass = nullptr; - - // Find the first klass in the CLDG. - while (cld != nullptr) { - assert_locked_or_safepoint(cld->metaspace_lock()); - klass = cld->_klasses; - if (klass != nullptr) { - _next_klass = klass; - return; - } - cld = cld->next(); - } + _cld = AtomicAccess::load_acquire(&ClassLoaderDataGraph::_head); } -Klass* ClassLoaderDataGraphKlassIteratorAtomic::next_klass_in_cldg(Klass* klass) { - Klass* next = klass->next_link(); - if (next != nullptr) { - return next; - } - - // No more klasses in the current CLD. Time to find a new CLD. - ClassLoaderData* cld = klass->class_loader_data(); - assert_locked_or_safepoint(cld->metaspace_lock()); - while (next == nullptr) { - cld = cld->next(); - if (cld == nullptr) { - break; +ClassLoaderData* ClassLoaderDataGraphIteratorAtomic::next() { + ClassLoaderData* cur = AtomicAccess::load(&_cld); + for (;;) { + if (cur == nullptr) { + return nullptr; } - next = cld->_klasses; - } - - return next; -} - -Klass* ClassLoaderDataGraphKlassIteratorAtomic::next_klass() { - Klass* head = _next_klass; - - while (head != nullptr) { - Klass* next = next_klass_in_cldg(head); - - Klass* old_head = AtomicAccess::cmpxchg(&_next_klass, head, next); - - if (old_head == head) { - return head; // Won the CAS. + ClassLoaderData* next = cur->next(); + ClassLoaderData* old; + if ((old = AtomicAccess::cmpxchg(&_cld, cur, next)) == cur) { + return cur; } - - head = old_head; + cur = old; } - - // Nothing more for the iterator to hand out. - assert(head == nullptr, "head is " PTR_FORMAT ", expected not null:", p2i(head)); - return nullptr; } void ClassLoaderDataGraph::verify() { diff --git a/src/hotspot/share/classfile/classLoaderDataGraph.hpp b/src/hotspot/share/classfile/classLoaderDataGraph.hpp index 1dcca4d1069..803f227dcf3 100644 --- a/src/hotspot/share/classfile/classLoaderDataGraph.hpp +++ b/src/hotspot/share/classfile/classLoaderDataGraph.hpp @@ -34,7 +34,7 @@ class ClassLoaderDataGraph : public AllStatic { friend class ClassLoaderData; - friend class ClassLoaderDataGraphKlassIteratorAtomic; + friend class ClassLoaderDataGraphIteratorAtomic; friend class VMStructs; private: class ClassLoaderDataGraphIterator; @@ -140,14 +140,14 @@ public: } }; -// An iterator that distributes Klasses to parallel worker threads. -class ClassLoaderDataGraphKlassIteratorAtomic : public StackObj { - Klass* volatile _next_klass; - public: - ClassLoaderDataGraphKlassIteratorAtomic(); - Klass* next_klass(); - private: - static Klass* next_klass_in_cldg(Klass* klass); +// An iterator that distributes Klasses to parallel worker threads based on CLDs. +class ClassLoaderDataGraphIteratorAtomic : public StackObj { + ClassLoaderData* volatile _cld; + +public: + ClassLoaderDataGraphIteratorAtomic(); + + ClassLoaderData* next(); }; #endif // SHARE_CLASSFILE_CLASSLOADERDATAGRAPH_HPP diff --git a/src/hotspot/share/gc/shared/parallelCleaning.cpp b/src/hotspot/share/gc/shared/parallelCleaning.cpp index 9d496783ca2..d2f69bfa679 100644 --- a/src/hotspot/share/gc/shared/parallelCleaning.cpp +++ b/src/hotspot/share/gc/shared/parallelCleaning.cpp @@ -27,7 +27,7 @@ #include "code/codeCache.hpp" #include "gc/shared/parallelCleaning.hpp" #include "logging/log.hpp" -#include "memory/resourceArea.hpp" +#include "oops/klass.inline.hpp" #include "runtime/atomicAccess.hpp" CodeCacheUnloadingTask::CodeCacheUnloadingTask(uint num_workers, bool unloading_occurred) : @@ -94,38 +94,26 @@ void CodeCacheUnloadingTask::work(uint worker_id) { } } -KlassCleaningTask::KlassCleaningTask() : - _clean_klass_tree_claimed(false), - _klass_iterator() { -} - -bool KlassCleaningTask::claim_clean_klass_tree_task() { - if (_clean_klass_tree_claimed) { - return false; - } - - return !AtomicAccess::cmpxchg(&_clean_klass_tree_claimed, false, true); -} - -InstanceKlass* KlassCleaningTask::claim_next_klass() { - Klass* klass; - do { - klass =_klass_iterator.next_klass(); - } while (klass != nullptr && !klass->is_instance_klass()); - - // this can be null so don't call InstanceKlass::cast - return static_cast(klass); -} - void KlassCleaningTask::work() { - // One worker will clean the subklass/sibling klass tree. - if (claim_clean_klass_tree_task()) { - Klass::clean_weak_klass_links(true /* class_unloading_occurred */, false /* clean_alive_klasses */); - } + for (ClassLoaderData* cur = _cld_iterator_atomic.next(); cur != nullptr; cur = _cld_iterator_atomic.next()) { + class CleanKlasses : public KlassClosure { + public: - // All workers will help cleaning the classes, - InstanceKlass* klass; - while ((klass = claim_next_klass()) != nullptr) { - Klass::clean_weak_instanceklass_links(klass); + void do_klass(Klass* klass) override { + klass->clean_subklass(true); + + Klass* sibling = klass->next_sibling(true); + klass->set_next_sibling(sibling); + + if (klass->is_instance_klass()) { + Klass::clean_weak_instanceklass_links(InstanceKlass::cast(klass)); + } + + assert(klass->subklass() == nullptr || klass->subklass()->is_loader_alive(), "must be"); + assert(klass->next_sibling(false) == nullptr || klass->next_sibling(false)->is_loader_alive(), "must be"); + } + } cl; + + cur->classes_do(&cl); } } diff --git a/src/hotspot/share/gc/shared/parallelCleaning.hpp b/src/hotspot/share/gc/shared/parallelCleaning.hpp index 4a7c724fef5..b47bf92e2ac 100644 --- a/src/hotspot/share/gc/shared/parallelCleaning.hpp +++ b/src/hotspot/share/gc/shared/parallelCleaning.hpp @@ -54,14 +54,10 @@ public: // Cleans out the Klass tree from stale data. class KlassCleaningTask : public StackObj { - volatile bool _clean_klass_tree_claimed; - ClassLoaderDataGraphKlassIteratorAtomic _klass_iterator; - - bool claim_clean_klass_tree_task(); - InstanceKlass* claim_next_klass(); + ClassLoaderDataGraphIteratorAtomic _cld_iterator_atomic; public: - KlassCleaningTask(); + KlassCleaningTask() : _cld_iterator_atomic() { } void work(); }; diff --git a/src/hotspot/share/oops/klass.cpp b/src/hotspot/share/oops/klass.cpp index a93875b86a5..b3386694b79 100644 --- a/src/hotspot/share/oops/klass.cpp +++ b/src/hotspot/share/oops/klass.cpp @@ -614,8 +614,7 @@ GrowableArray* Klass::compute_secondary_supers(int num_extra_slots, // subklass links. Used by the compiler (and vtable initialization) // May be cleaned concurrently, so must use the Compile_lock. -// The log parameter is for clean_weak_klass_links to report unlinked classes. -Klass* Klass::subklass(bool log) const { +Klass* Klass::subklass() const { // Need load_acquire on the _subklass, because it races with inserts that // publishes freshly initialized data. for (Klass* chain = AtomicAccess::load_acquire(&_subklass); @@ -626,11 +625,6 @@ Klass* Klass::subklass(bool log) const { { if (chain->is_loader_alive()) { return chain; - } else if (log) { - if (log_is_enabled(Trace, class, unload)) { - ResourceMark rm; - log_trace(class, unload)("unlinking class (subclass): %s", chain->external_name()); - } } } return nullptr; @@ -701,15 +695,20 @@ void Klass::append_to_sibling_list() { DEBUG_ONLY(verify();) } -void Klass::clean_subklass() { +// The log parameter is for clean_weak_klass_links to report unlinked classes. +Klass* Klass::clean_subklass(bool log) { for (;;) { // Need load_acquire, due to contending with concurrent inserts Klass* subklass = AtomicAccess::load_acquire(&_subklass); if (subklass == nullptr || subklass->is_loader_alive()) { - return; + return subklass; + } + if (log && log_is_enabled(Trace, class, unload)) { + ResourceMark rm; + log_trace(class, unload)("unlinking class (subclass): %s", subklass->external_name()); } // Try to fix _subklass until it points at something not dead. - AtomicAccess::cmpxchg(&_subklass, subklass, subklass->next_sibling()); + AtomicAccess::cmpxchg(&_subklass, subklass, subklass->next_sibling(log)); } } @@ -728,8 +727,7 @@ void Klass::clean_weak_klass_links(bool unloading_occurred, bool clean_alive_kla assert(current->is_loader_alive(), "just checking, this should be live"); // Find and set the first alive subklass - Klass* sub = current->subklass(true); - current->clean_subklass(); + Klass* sub = current->clean_subklass(true); if (sub != nullptr) { stack.push(sub); } diff --git a/src/hotspot/share/oops/klass.hpp b/src/hotspot/share/oops/klass.hpp index 70d9ce3a881..db3360b080e 100644 --- a/src/hotspot/share/oops/klass.hpp +++ b/src/hotspot/share/oops/klass.hpp @@ -300,7 +300,7 @@ protected: // Use InstanceKlass::contains_field_offset to classify field offsets. // sub/superklass links - Klass* subklass(bool log = false) const; + Klass* subklass() const; Klass* next_sibling(bool log = false) const; void append_to_sibling_list(); // add newly created receiver to superklass' subklass list @@ -413,9 +413,9 @@ protected: virtual ModuleEntry* module() const = 0; virtual PackageEntry* package() const = 0; + void set_next_sibling(Klass* s); protected: // internal accessors void set_subklass(Klass* s); - void set_next_sibling(Klass* s); private: static uint8_t compute_hash_slot(Symbol* s); @@ -743,7 +743,7 @@ public: inline bool is_loader_alive() const; inline bool is_loader_present_and_alive() const; - void clean_subklass(); + Klass* clean_subklass(bool log = false); // Clean out unnecessary weak klass links from the whole klass hierarchy. static void clean_weak_klass_links(bool unloading_occurred, bool clean_alive_klasses = true);