From 991ac9e6168b2573f78772e2d7936792a43fe336 Mon Sep 17 00:00:00 2001 From: Igor Veresov Date: Tue, 2 Sep 2025 21:28:22 +0000 Subject: [PATCH] 8365407: Race condition in MethodTrainingData::verify() Reviewed-by: kvn, vlivanov, iklam --- .../share/compiler/compilationPolicy.cpp | 24 ++++---- .../share/compiler/compilationPolicy.hpp | 26 ++++---- src/hotspot/share/oops/trainingData.cpp | 59 ++++++++++++------- src/hotspot/share/oops/trainingData.hpp | 10 ++-- src/hotspot/share/runtime/init.cpp | 5 +- src/hotspot/share/runtime/java.cpp | 7 +++ 6 files changed, 73 insertions(+), 58 deletions(-) diff --git a/src/hotspot/share/compiler/compilationPolicy.cpp b/src/hotspot/share/compiler/compilationPolicy.cpp index 9c49d941bbc..80223a4e5cb 100644 --- a/src/hotspot/share/compiler/compilationPolicy.cpp +++ b/src/hotspot/share/compiler/compilationPolicy.cpp @@ -138,7 +138,7 @@ void CompilationPolicy::compile_if_required(const methodHandle& m, TRAPS) { } } -void CompilationPolicy::replay_training_at_init_impl(InstanceKlass* klass, TRAPS) { +void CompilationPolicy::replay_training_at_init_impl(InstanceKlass* klass, JavaThread* current) { if (!klass->has_init_deps_processed()) { ResourceMark rm; log_debug(training)("Replay training: %s", klass->external_name()); @@ -150,11 +150,11 @@ void CompilationPolicy::replay_training_at_init_impl(InstanceKlass* klass, TRAPS assert(klass->has_init_deps_processed(), ""); if (AOTCompileEagerly) { ktd->iterate_comp_deps([&](CompileTrainingData* ctd) { - if (ctd->init_deps_left() == 0) { + if (ctd->init_deps_left_acquire() == 0) { MethodTrainingData* mtd = ctd->method(); if (mtd->has_holder()) { - const methodHandle mh(THREAD, const_cast(mtd->holder())); - CompilationPolicy::maybe_compile_early(mh, THREAD); + const methodHandle mh(current, const_cast(mtd->holder())); + CompilationPolicy::maybe_compile_early(mh, current); } } }); @@ -163,10 +163,10 @@ void CompilationPolicy::replay_training_at_init_impl(InstanceKlass* klass, TRAPS } } -void CompilationPolicy::replay_training_at_init(InstanceKlass* klass, TRAPS) { +void CompilationPolicy::replay_training_at_init(InstanceKlass* klass, JavaThread* current) { assert(klass->is_initialized(), ""); if (TrainingData::have_data() && klass->is_shared()) { - _training_replay_queue.push(klass, TrainingReplayQueue_lock, THREAD); + _training_replay_queue.push(klass, TrainingReplayQueue_lock, current); } } @@ -181,11 +181,11 @@ void CompilationPolicyUtils::Queue::print_on(outputStream* st) { } } -void CompilationPolicy::replay_training_at_init_loop(TRAPS) { +void CompilationPolicy::replay_training_at_init_loop(JavaThread* current) { while (!CompileBroker::is_compilation_disabled_forever()) { - InstanceKlass* ik = _training_replay_queue.pop(TrainingReplayQueue_lock, THREAD); + InstanceKlass* ik = _training_replay_queue.pop(TrainingReplayQueue_lock, current); if (ik != nullptr) { - replay_training_at_init_impl(ik, THREAD); + replay_training_at_init_impl(ik, current); } } } @@ -446,7 +446,7 @@ void CompilationPolicy::print_training_data_on(outputStream* st, const char* pr if (ctd == nullptr) { st->print("null"); } else { - st->print("%d", ctd->init_deps_left()); + st->print("%d", ctd->init_deps_left_acquire()); } } } @@ -1172,7 +1172,7 @@ CompLevel CompilationPolicy::trained_transition_from_none(const methodHandle& me CompileTrainingData* ctd = mtd->last_toplevel_compile(CompLevel_full_optimization); assert(ctd != nullptr, "Should have CTD for CompLevel_full_optimization"); // With SkipTier2IfPossible and all deps satisfied, go to level 4 immediately - if (SkipTier2IfPossible && ctd->init_deps_left() == 0) { + if (SkipTier2IfPossible && ctd->init_deps_left_acquire() == 0) { if (method->method_data() == nullptr) { create_mdo(method, THREAD); } @@ -1200,7 +1200,7 @@ CompLevel CompilationPolicy::trained_transition_from_limited_profile(const metho assert(training_has_profile, "Have to have a profile to be here"); // Check if the method is ready CompileTrainingData* ctd = mtd->last_toplevel_compile(CompLevel_full_optimization); - if (ctd != nullptr && ctd->init_deps_left() == 0) { + if (ctd != nullptr && ctd->init_deps_left_acquire() == 0) { if (method->method_data() == nullptr) { create_mdo(method, THREAD); } diff --git a/src/hotspot/share/compiler/compilationPolicy.hpp b/src/hotspot/share/compiler/compilationPolicy.hpp index f4a7c4c249b..d950ba418f9 100644 --- a/src/hotspot/share/compiler/compilationPolicy.hpp +++ b/src/hotspot/share/compiler/compilationPolicy.hpp @@ -74,32 +74,28 @@ class Queue { } public: Queue() : _head(nullptr), _tail(nullptr) { } - void push(T* value, Monitor* lock, TRAPS) { - MonitorLocker locker(THREAD, lock); + void push(T* value, Monitor* lock, JavaThread* current) { + MonitorLocker locker(current, lock); push_unlocked(value); locker.notify_all(); } bool is_empty_unlocked() const { return _head == nullptr; } - T* pop(Monitor* lock, TRAPS) { - MonitorLocker locker(THREAD, lock); - while(is_empty_unlocked() && !CompileBroker::is_compilation_disabled_forever()) { + T* pop(Monitor* lock, JavaThread* current) { + MonitorLocker locker(current, lock); + while (is_empty_unlocked() && !CompileBroker::is_compilation_disabled_forever()) { locker.wait(); } T* value = pop_unlocked(); return value; } - T* try_pop(Monitor* lock, TRAPS) { - MonitorLocker locker(THREAD, lock); - T* value = nullptr; - if (!is_empty_unlocked()) { - value = pop_unlocked(); - } + T* try_pop(Monitor* lock, JavaThread* current) { + MonitorLocker locker(current, lock); + T* value = pop_unlocked(); return value; } - void print_on(outputStream* st); }; } // namespace CompilationPolicyUtils @@ -342,7 +338,7 @@ class CompilationPolicy : AllStatic { // m must be compiled before executing it static bool must_be_compiled(const methodHandle& m, int comp_level = CompLevel_any); static void maybe_compile_early(const methodHandle& m, TRAPS); - static void replay_training_at_init_impl(InstanceKlass* klass, TRAPS); + static void replay_training_at_init_impl(InstanceKlass* klass, JavaThread* current); public: static int min_invocations() { return Tier4MinInvocationThreshold; } static int c1_count() { return _c1_count; } @@ -352,8 +348,8 @@ class CompilationPolicy : AllStatic { // This supports the -Xcomp option. static void compile_if_required(const methodHandle& m, TRAPS); - static void replay_training_at_init(InstanceKlass* klass, TRAPS); - static void replay_training_at_init_loop(TRAPS); + static void replay_training_at_init(InstanceKlass* klass, JavaThread* current); + static void replay_training_at_init_loop(JavaThread* current); // m is allowed to be compiled static bool can_be_compiled(const methodHandle& m, int comp_level = CompLevel_any); diff --git a/src/hotspot/share/oops/trainingData.cpp b/src/hotspot/share/oops/trainingData.cpp index 70e8f2437c1..4797eed0a31 100644 --- a/src/hotspot/share/oops/trainingData.cpp +++ b/src/hotspot/share/oops/trainingData.cpp @@ -83,7 +83,7 @@ static void verify_archived_entry(TrainingData* td, const TrainingData::Key* k) } void TrainingData::verify() { - if (TrainingData::have_data()) { + if (TrainingData::have_data() && !TrainingData::assembling_data()) { archived_training_data_dictionary()->iterate([&](TrainingData* td) { if (td->is_KlassTrainingData()) { KlassTrainingData* ktd = td->as_KlassTrainingData(); @@ -98,9 +98,21 @@ void TrainingData::verify() { Key k(mtd->holder()); verify_archived_entry(td, &k); } - mtd->verify(); - } else if (td->is_CompileTrainingData()) { - td->as_CompileTrainingData()->verify(); + mtd->verify(/*verify_dep_counter*/true); + } + }); + } + if (TrainingData::need_data()) { + TrainingDataLocker l; + training_data_set()->iterate([&](TrainingData* td) { + if (td->is_KlassTrainingData()) { + KlassTrainingData* ktd = td->as_KlassTrainingData(); + ktd->verify(); + } else if (td->is_MethodTrainingData()) { + MethodTrainingData* mtd = td->as_MethodTrainingData(); + // During the training run init deps tracking is not setup yet, + // don't verify it. + mtd->verify(/*verify_dep_counter*/false); } }); } @@ -229,7 +241,7 @@ CompileTrainingData* CompileTrainingData::make(CompileTask* task) { } -void CompileTrainingData::dec_init_deps_left(KlassTrainingData* ktd) { +void CompileTrainingData::dec_init_deps_left_release(KlassTrainingData* ktd) { LogStreamHandle(Trace, training) log; if (log.is_enabled()) { log.print("CTD "); print_on(&log); log.cr(); @@ -450,7 +462,7 @@ void KlassTrainingData::notice_fully_initialized() { TrainingDataLocker l; // Not a real lock if we don't collect the data, // that's why we need the atomic decrement below. for (int i = 0; i < comp_dep_count(); i++) { - comp_dep(i)->dec_init_deps_left(this); + comp_dep(i)->dec_init_deps_left_release(this); } holder()->set_has_init_deps_processed(); } @@ -476,10 +488,10 @@ void TrainingData::init_dumptime_table(TRAPS) { _dumptime_training_data_dictionary->append(td); } }); + } - if (AOTVerifyTrainingData) { - training_data_set()->verify(); - } + if (AOTVerifyTrainingData) { + TrainingData::verify(); } } @@ -592,22 +604,13 @@ void KlassTrainingData::verify() { } } -void MethodTrainingData::verify() { - iterate_compiles([](CompileTrainingData* ctd) { - ctd->verify(); - - int init_deps_left1 = ctd->init_deps_left(); - int init_deps_left2 = ctd->compute_init_deps_left(); - - if (init_deps_left1 != init_deps_left2) { - ctd->print_on(tty); tty->cr(); - } - guarantee(init_deps_left1 == init_deps_left2, "mismatch: %d %d %d", - init_deps_left1, init_deps_left2, ctd->init_deps_left()); +void MethodTrainingData::verify(bool verify_dep_counter) { + iterate_compiles([&](CompileTrainingData* ctd) { + ctd->verify(verify_dep_counter); }); } -void CompileTrainingData::verify() { +void CompileTrainingData::verify(bool verify_dep_counter) { for (int i = 0; i < init_dep_count(); i++) { KlassTrainingData* ktd = init_dep(i); if (ktd->has_holder() && ktd->holder()->defined_by_other_loaders()) { @@ -624,6 +627,18 @@ void CompileTrainingData::verify() { } guarantee(ktd->_comp_deps.contains(this), ""); } + + if (verify_dep_counter) { + int init_deps_left1 = init_deps_left_acquire(); + int init_deps_left2 = compute_init_deps_left(); + + bool invariant = (init_deps_left1 >= init_deps_left2); + if (!invariant) { + print_on(tty); + tty->cr(); + } + guarantee(invariant, "init deps invariant violation: %d >= %d", init_deps_left1, init_deps_left2); + } } void CompileTrainingData::cleanup(Visitor& visitor) { diff --git a/src/hotspot/share/oops/trainingData.hpp b/src/hotspot/share/oops/trainingData.hpp index c47d0a0f66e..b909be12324 100644 --- a/src/hotspot/share/oops/trainingData.hpp +++ b/src/hotspot/share/oops/trainingData.hpp @@ -673,9 +673,9 @@ public: } _init_deps.clear(); } - void dec_init_deps_left(KlassTrainingData* ktd); - int init_deps_left() const { - return Atomic::load(&_init_deps_left); + void dec_init_deps_left_release(KlassTrainingData* ktd); + int init_deps_left_acquire() const { + return Atomic::load_acquire(&_init_deps_left); } uint compute_init_deps_left(bool count_initialized = false); @@ -707,7 +707,7 @@ public: return (int)align_metadata_size(align_up(sizeof(CompileTrainingData), BytesPerWord)/BytesPerWord); } - void verify(); + void verify(bool verify_dep_counter); static CompileTrainingData* allocate(MethodTrainingData* mtd, int level, int compile_id) { return TrainingData::allocate(mtd, level, compile_id); @@ -828,7 +828,7 @@ class MethodTrainingData : public TrainingData { return "{ method training data }"; }; - void verify(); + void verify(bool verify_dep_counter); static MethodTrainingData* allocate(Method* m, KlassTrainingData* ktd) { return TrainingData::allocate(m, ktd); diff --git a/src/hotspot/share/runtime/init.cpp b/src/hotspot/share/runtime/init.cpp index 73292a7b6d9..56e6ea30c0a 100644 --- a/src/hotspot/share/runtime/init.cpp +++ b/src/hotspot/share/runtime/init.cpp @@ -195,10 +195,7 @@ jint init_globals2() { } #endif - // Initialize TrainingData only we're recording/replaying - if (TrainingData::have_data() || TrainingData::need_data()) { - TrainingData::initialize(); - } + TrainingData::initialize(); if (!universe_post_init()) { return JNI_ERR; diff --git a/src/hotspot/share/runtime/java.cpp b/src/hotspot/share/runtime/java.cpp index b3b46e5a9ab..351fd1ebb89 100644 --- a/src/hotspot/share/runtime/java.cpp +++ b/src/hotspot/share/runtime/java.cpp @@ -34,6 +34,7 @@ #include "classfile/systemDictionary.hpp" #include "code/codeCache.hpp" #include "compiler/compilationMemoryStatistic.hpp" +#include "compiler/compilationPolicy.hpp" #include "compiler/compileBroker.hpp" #include "compiler/compilerOracle.hpp" #include "gc/shared/collectedHeap.hpp" @@ -503,6 +504,12 @@ void before_exit(JavaThread* thread, bool halt) { // Note: we don't wait until it actually dies. os::terminate_signal_thread(); + #if INCLUDE_CDS + if (AOTVerifyTrainingData) { + TrainingData::verify(); + } + #endif + print_statistics(); { MutexLocker ml(BeforeExit_lock);