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

[Coleen Phillimore]

On Thu, 10 Jul 2025 08:35:48 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Use OopStorage directly instead of JNI handles. Note that we never destroy TrainingData objects, so we don't need to concern ourselves with freeing the OopStorage entries. Also, keeping the klasses alive is only necessary during the training run. During the replay the klasses TD objects refer to are always alive.
>
> 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.

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

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