On Mon, 15 Mar 2021 21:25:30 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> JavaValue is a small wrapper class that wraps values used to pass arguments >> and results between native and Java. >> >> When JavaCalls::call returns an object, the value stored in the JavaValue is >> not a handliezed jobject. Instead it's a raw oop. So, most of the code >> handling the `result`, fetches the result as a jobject, and then immediately >> casts it to an oop. For example: >> oop res = (oop)result.get_jobject(); >> >> I'd like to change this code to be: >> oop res = result.get_oop(); >> >> The motivations for this patch is: >> >> 1) Minimize the places where we pass around oops in jobject variables. Maybe >> at some point we'll have converted the JVM to only use the jobject type when >> passing around JNI handle. We need to be stricter with the types when we >> continue develop our GCs and their barriers. >> >> 2) Limit the number of places in the code where we perform raw oop casts. We >> have a helper cast function for that, cast_to_oop, but not all code use it. >> I have future patches where the compiler will completely forbid raw cast to >> oops (in fastdebug builds). With that in place, I can then add more stricter >> oop verification code when oops are created. This helps catching bugs >> earlier. >> >> --- >> >> When reviewing this patch, take an extra look at the change to >> oopsHierarchy.hpp. This was done to support jvmciEnv.cpp code: >> JVMCIObject wrap(oop obj)... >> JVMCIObjectArray wrap(objArrayOop obj)... >> JVMCIPrimitiveArray wrap(typeArrayOop obj) ... >> Previously, `wrap((oop)result.get_jobject())` called the first function. >> When the code was changed to `wrap(result.get_oop())`, where `get_oop()` >> returns a `oopDesc*`, the compiler didn't know what conversion in >> oopsHierarchy.hpp to use. Therefore, I replaced the overly permissive >> `void*` constructor with a constructor that only takes the corresponding >> `type##OopDesc*`. >> >> An alternative would be to let get_oop() return an oop, but then that would >> add an unwanted a dependency between globalDefinitions.hpp and >> oopsHierarchy.hpp. An earlier version of this patch did return an oop >> instead of oopDesc*, but it also moved entire JavaValue class out of >> globalDefinitions.hpp into a new javaValue.hpp file, and had a corresponding >> javaValue.inline.hpp file. >> >> Even if we end up using the proposed `oopDesc* get_oop()` version, maybe >> moving the class to javaValues.hpp would still makes sense? > > src/hotspot/share/utilities/globalDefinitions.hpp line 809: > >> 807: jint i; >> 808: jlong l; >> 809: jobject h; > > Do we still need jobject after this change? We still use jobject for the arguments. We also converted the result to a jobject when it is passed back to Java. We have a few places in JFR where the code seems to take a detour and fetch the oop from the result JavaValue, create a JNI handle, puts it back with set_jobject, and then reads it out with get_jobject. I have a patch where the code simply returns the created JNI handle without installing it into the result JavaValue. I've left that as a separate patch for the JFR team to review. There are some usages in C1, but I haven't tried to figure that out. ------------- PR: https://git.openjdk.java.net/jdk/pull/3013
