RFR: 8358580: Rethink how classes are kept alive in training data

[Igor Veresov]

On Thu, 10 Jul 2025 11:51:34 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/oops/trainingData.cpp line 437:
>> 
>>> 435: KlassTrainingData::KlassTrainingData(InstanceKlass* klass) : TrainingData(klass) {
>>> 436:   assert(klass != nullptr, "");
>>> 437:   oop* handle = oop_storage()->allocate();
>> 
>> I don't think you are supposed to allocate from `OopStorage` directly, that's the job for various `Handle`-s. Also, capturing the `java_mirror` does not really block the unloading, see:
>> 
>> 
>> // Loading the java_mirror does not keep its holder alive. See Klass::keep_alive().
>> inline oop Klass::java_mirror() const {
>>   return _java_mirror.resolve();
>> }
>> 
>> 
>> So the idiomatic way would be:
>> 
>> 
>> _holder_mirror = OopHandle(Universe::vm_global(), klass->klass_holder());
>
> What a confusing comment, but luckily it points to Klass::keep_alive() for context.  Yes, please don't allocate an OopStorage handle directly.  Then the OopHandle constructor will check for native oom.
> 
> Otherwise this seems okay and better than using jni.

@coleenp I kind of like the fact that I can get rid of a field in KTD that previously stored a handle. Perhaps that benefit justifies a direct allocation?

@shipilev I don't understand your comment about loading the mirror. I'm not merely loading it, I'm registering it as a root. It is also all happening in a safepoint-free context.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26233#discussion_r2197939820