RFR: 8365407: Race condition in MethodTrainingData::verify() [v2]

[David Holmes]

On Wed, 20 Aug 2025 18:32:22 GMT, Igor Veresov <iveresov at openjdk.org> wrote:

>> This change fixes multiple issue with training data verification. While the current state of things in the mainline will not cause  any issues (because of the absence of the call to `TD::verify()` during the shutdown) it does problems in the leyden repo. This change strengthens verification in the mainline (by adding the shutdown verify call), and fixes the problems that prevent  it from working reliably.
>
> Igor Veresov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix minimal build

Just a few drive-by comments as I'm not familiar with this "training" stuff.

src/hotspot/share/compiler/compilationPolicy.cpp line 141:

> 139: }
> 140: 
> 141:  void CompilationPolicy::flush_replay_training_at_init(TRAPS) {

This method seems to be waiting for something to finish, not "flushing" anything itself.

src/hotspot/share/compiler/compilationPolicy.cpp line 142:

> 140: 
> 141:  void CompilationPolicy::flush_replay_training_at_init(TRAPS) {
> 142:     MonitorLocker locker(THREAD, TrainingReplayQueue_lock);

There is no exception processing here so this method should not be declared to take `TRAPS`. If you want to pass the current thread just declare a `JavaThread* current` parameter directly please.

src/hotspot/share/oops/trainingData.hpp line 678:

> 676:   void dec_init_deps_left(KlassTrainingData* ktd);
> 677:   int init_deps_left() const {
> 678:     return Atomic::load_acquire(&_init_deps_left);

Where is the `release_store` (or other ordered atomic op) that this pairs with?

Also there is a convention to make acquire/release semantics clear in the API method names i.e. in this case `init_deps_left_acquire()`.

src/hotspot/share/runtime/java.cpp line 522:

> 520:   if (AOTVerifyTrainingData) {
> 521:     EXCEPTION_MARK;
> 522:     CompilationPolicy::flush_replay_training_at_init(THREAD);

Looks odd to have an `at_init` method executing during VM shutdown.

-------------

PR Review: https://git.openjdk.org/jdk/pull/26866#pullrequestreview-3138788035
PR Review Comment: https://git.openjdk.org/jdk/pull/26866#discussion_r2289684061
PR Review Comment: https://git.openjdk.org/jdk/pull/26866#discussion_r2289682761
PR Review Comment: https://git.openjdk.org/jdk/pull/26866#discussion_r2289698544
PR Review Comment: https://git.openjdk.org/jdk/pull/26866#discussion_r2289701195