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

[David Holmes]

On Thu, 21 Aug 2025 02:27:40 GMT, Igor Veresov <iveresov at openjdk.org> wrote:

>> 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()`.
>
> There is an `Atomic::sub()` in `dec_init_deps_left()`.

Okay - not obvious we actually require acquire semantics when reading a simple count, but I'm not sure what the count may imply. But please consider renaming the method.

>> 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.
>
> `init` here means `class initialization`. We can rename it if you want, but that's the current convention everybody expects in the leyden project.

"from_init" might be better if this represents training data from class initialization time. Though is there non-init training data? i.e. could it just be `flush_replay_training`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26866#discussion_r2289754815
PR Review Comment: https://git.openjdk.org/jdk/pull/26866#discussion_r2289756627