On Tue, 9 Sep 2025 05:21:10 GMT, Ioi Lam <[email protected]> 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); > > Klass* get_klass_considering_redefinition(jclass cls, JavaThread *thread); > InstanceKlass* get_instance_klass_considering_redefinition(jclass cls, > JavaThread *thread); > > > Notes: > > [1] 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()`. > > [2] I refactored > `JvmtiThreadState::class_to_verify_considering_redefinition()` so that the > caller of this funcation can avoid using `InstanceKlass::cast()`. This is > possible because we ONLY store `InstanceKlass*` in > `JvmtiThreadState::set_class_being_redefined()` > > I also removed a few cases of unnecessary `InstanceKlass::cast()`.
Sorry but I think this PR is trying to do too many things at once. It is pushing JNI resolving inside internal JVM methods, which I think is a bad thing - we resolve JNI references at the boundary to get the oop that the VM wants to work with. Internal APIs should be oblivious to jobject and friends IMO. Also there may be times that the JNI/JVM method needs get the oop itself before extracting the klass. You are converting klass to instanceKlass where it has to be instanceKlass e.g. with redefinition. This is a good thing, but it is a very distinct thing that deserves its own cleanup (as per previous changes in that area). You are defining a helper `java_lang_Class::as_InstanceKlass` to internalize the cast - this is fine but again a simple cleanup that would be better standalone IMO. It is also not clear that JVM/JNI API's are properly checking that the incoming jobject is in fact a class of the right kind (ie not an array class object). ------------- Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/27158#pullrequestreview-3199393023
