RFR: 8340586: JdkJfrEvent::get_all_klasses stores non-strong oops in JNI handles

[Stefan Karlsson]

On Tue, 5 Nov 2024 13:12:51 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> JdkJfrEvent::get_all_klasses walks the class hierarchy of the Event class to find all event sub klasses. The mirrors of these classes are then stored in JNI handles and eventually transfered to an arraylist which is returned to the java caller.
>> 
>> Because the klasses are found by walking the class hierarchy, their is no guarantee that the klass is strongly reachable, and thus that the java_mirror load is strong. So we must explicitly keep the klass alive (and not have a safepoint between the load and the store into the object graph).
>> 
>> Having both `fill_klasses` and `transform_klasses_to_local_jni_handles` made the call to `keep_alive` and the comment explaining the reason why somewhat obscured, so I merged them. (The split seems to have served no purpose except to introduce the need for a lot of const and reinterpret casts.)
>
> src/hotspot/share/oops/klass.inline.hpp line 44:
> 
>> 42:   static_cast<void>(klass_holder());
>> 43: }
>> 44: 
> 
> Can you add a comment to Klass::java_mirror() that says it doesn't keep the loader alive, "see above".

> safe to store in the object graph.

It could be good to also mention roots like Handles, since that seems to be a common misunderstanding:

> safe to store in the object graph and it's roots (E.g. Handles).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21893#discussion_r1829360474