RFR: 8367142: Simplify java mirror handling in JNI methods [v2]

[David Holmes]

On Tue, 9 Sep 2025 22:09:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> The purpose of this PR is to simplify JNI code and also to avoid unnecessary `InstanceKlass::cast()` calls.
>> 
>> This PR is intended to be a strict clean-up that preserves existing behaviors.
>> 
>> The following helper functions are added to simplify boilerplate code in JNI methods.
>> 
>> 
>> static Klass* java_lang_Class::as_Klass(jobject java_class);
>> static InstanceKlass* java_lang_Class::as_InstanceKlass(oop java_class);
>> static InstanceKlass* java_lang_Class::as_InstanceKlass(jobject java_class);
>> 
>> 
>> Note:
>> 
>> Before this PR, we have both patterns:
>> 
>> 
>> java_lang_Class::as_Klass(JNIHandles::resolve(cls));
>> java_lang_Class::as_Klass(JNIHandles::resolve_non_null(cls));
>> 
>> 
>> If `cls` is null, we would get an assert in both cases (`as_Klass()` requires a non-null input). Therefore, I am using `resolve_non_null()` in the `jobject` versions of `as_Klass()`.
>
> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
> 
>  - Merge branch 'master' into 8367142-simplify-java-mirror-handling-in-jni-methods
>  - @dholmes-ora comments - remove class_to_verify_considering_redefinition() changes, to be done in separate PR
>  - more fixes
>  - tmp: Clean up java mirror handling in JNI methods

Looks good. A couple of minor nits.

Thanks

src/hotspot/share/prims/jvm.cpp line 844:

> 842:     ResourceMark rm;
> 843:     const char * from_name = java_lang_Class::as_Klass(from)->external_name();
> 844:     const char * to_name = java_lang_Class::as_Klass(result)->external_name();

Suggestion:

    const char* from_name = java_lang_Class::as_Klass(from)->external_name();
    const char* to_name = java_lang_Class::as_Klass(result)->external_name();

pre-existing nit

src/hotspot/share/prims/jvm.cpp line 910:

> 908: 
> 909:   InstanceKlass* lookup_k = java_lang_Class::as_InstanceKlass(lookup);
> 910:   // Lookup class must not be a primitive class (whose mirror null Klass*)

Suggestion:

  // Lookup class must not be a primitive class (whose mirror is a null Klass*)

src/hotspot/share/prims/jvm.cpp line 912:

> 910:   // Lookup class must not be a primitive class (whose mirror null Klass*)
> 911:   if (lookup_k == nullptr) {
> 912:     THROW_MSG_NULL(vmSymbols::java_lang_IllegalArgumentException(), "Lookup class is primitive");

This is a behavioural change.

src/hotspot/share/prims/whitebox.cpp line 2166:

> 2164: 
> 2165: WB_ENTRY(void, WB_LinkClass(JNIEnv* env, jobject wb, jclass clazz))
> 2166:   Klass *k = java_lang_Class::as_Klass(clazz);

Suggestion:

  Klass* k = java_lang_Class::as_Klass(clazz);

src/hotspot/share/prims/whitebox.cpp line 2168:

> 2166:   Klass *k = java_lang_Class::as_Klass(clazz);
> 2167:   if (k->is_instance_klass()) {
> 2168:     InstanceKlass *ik = InstanceKlass::cast(k);

Suggestion:

    InstanceKlass* ik = InstanceKlass::cast(k);

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27158#pullrequestreview-3205473716
PR Review Comment: https://git.openjdk.org/jdk/pull/27158#discussion_r2336277816
PR Review Comment: https://git.openjdk.org/jdk/pull/27158#discussion_r2336280699
PR Review Comment: https://git.openjdk.org/jdk/pull/27158#discussion_r2336282079
PR Review Comment: https://git.openjdk.org/jdk/pull/27158#discussion_r2336308779
PR Review Comment: https://git.openjdk.org/jdk/pull/27158#discussion_r2336309246