8365407: Race condition in MethodTrainingData::verify()

src/hotspot/share/compiler/compilationPolicy.cpp

@@ -138,7 +138,14 @@ void CompilationPolicy::compile_if_required(const methodHandle& m, TRAPS) {
   }
 }
 
-void CompilationPolicy::replay_training_at_init_impl(InstanceKlass* klass, TRAPS) {
+ void CompilationPolicy::wait_replay_training_to_finish(JavaThread* current) {
+    MonitorLocker locker(current, TrainingReplayQueue_lock);
+    while (!_training_replay_queue.is_empty_unlocked() || _training_replay_queue.is_processing_unlocked()) {
+      locker.wait(); // let the replay training thread drain the queue
+    }
+ }
+
+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 +157,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<Method*>(mtd->holder()));
-              CompilationPolicy::maybe_compile_early(mh, THREAD);
+              const methodHandle mh(current, const_cast<Method*>(mtd->holder()));
+              CompilationPolicy::maybe_compile_early(mh, current);
             }
           }
         });
@@ -163,10 +170,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 +188,12 @@ void CompilationPolicyUtils::Queue<InstanceKlass>::print_on(outputStream* st) {
   }
 }
 
-void CompilationPolicy::replay_training_at_init_loop(TRAPS) {
-  while (!CompileBroker::is_compilation_disabled_forever()) {
-    InstanceKlass* ik = _training_replay_queue.pop(TrainingReplayQueue_lock, THREAD);
+void CompilationPolicy::replay_training_at_init_loop(JavaThread* current) {
+  while (!CompileBroker::is_compilation_disabled_forever() || AOTVerifyTrainingData) {
+    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);
+      _training_replay_queue.processing_done(TrainingReplayQueue_lock, current);
     }
   }
 }
@@ -446,7 +454,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 +1180,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 +1208,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);
     }

src/hotspot/share/compiler/compilationPolicy.hpp

@@ -46,6 +46,7 @@ class Queue {
 
   QueueNode* _head;
   QueueNode* _tail;
+  int _processing;
 
   void push_unlocked(T* value) {
     QueueNode* n = new QueueNode(value, nullptr);
@@ -68,38 +69,47 @@ class Queue {
     T* value = nullptr;
     if (n != nullptr) {
       value = n->value();
+      ++_processing;
       delete n;
     }
     return value;
   }
+  void processing_done_unlocked() {
+    precond(_processing > 0);
+    --_processing;
+  }
 public:
-  Queue() : _head(nullptr), _tail(nullptr) { }
-  void push(T* value, Monitor* lock, TRAPS) {
-    MonitorLocker locker(THREAD, lock);
+  Queue() : _head(nullptr), _tail(nullptr), _processing(0) { }
+  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; }
+  bool is_processing_unlocked() const { return _processing > 0; }
 
-  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() || AOTVerifyTrainingData)) {
       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 processing_done(Monitor* lock, JavaThread* current) {
+    MonitorLocker locker(current, lock);
+    processing_done_unlocked();
+    locker.notify_all();
+  }
+
   void print_on(outputStream* st);
 };
 } // namespace CompilationPolicyUtils
@@ -342,7 +352,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 +362,9 @@ 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 wait_replay_training_to_finish(JavaThread* current);
+  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);

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();
+
+    if (init_deps_left1 != init_deps_left2) {
+      print_on(tty);
+      tty->cr();
+    }
+    guarantee(init_deps_left1 == init_deps_left2,
+              "init deps invariant violation: %d == %d", init_deps_left1, init_deps_left2);
+  }
 }
 
 void CompileTrainingData::cleanup(Visitor& visitor) {

src/hotspot/share/oops/trainingData.hpp

@@ -673,9 +673,9 @@ class CompileTrainingData : public TrainingData {
     }
     _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 @@ class CompileTrainingData : public TrainingData {
     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<CompileTrainingData>(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<MethodTrainingData>(m, ktd);

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;

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"
@@ -515,6 +516,14 @@ 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) {
+    EXCEPTION_MARK;
+    CompilationPolicy::wait_replay_training_to_finish(THREAD);
+    TrainingData::verify();
+  }
+  #endif
+
   print_statistics();
 
   { MutexLocker ml(BeforeExit_lock);