Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold, On 1/06/2020 8:57 am, Harold Seigel wrote: Thanks for the comments. Here's version 3 of the JDK and VM changes for sealed classes. full webrev: http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/ The new webrev contains just the following three changes: 1. The sealed classes API's in Class.java (permittedSubclasses() and isSealed()) were revised and, in particular, API permittedSubclasses() no longer uses reflection. For those following along we have presently abandoned the attempt to cache the array in ReflectionData. Current changes look okay. But I note from the CSR there appears to be a further minor update to the javadoc coming. 2. An unneeded 'if' statement was removed from JVM_GetPermittedSubclasses() (pointed out by David.) Looks good. 3. VM runtime test files SealedUnnamedModuleIntfTest.java and Permitted.java were changed to add a test case for a non-public permitted subclass and its sealed superclass being in the same module and package. Looks good. Additionally, two follow on RFE's will be filed. One to add additional VM sealed classes tests Thanks. I think there is a more mechanical approach to testing here that will allow the complete matrix to be easily covered with minimal duplication between testing for named and unnamed modules. and one to improve the implementations of the sealed classes API's in Class.java. Thanks. David - Thanks, Harold On 5/28/2020 8:30 PM, David Holmes wrote: Hi Harold, Sorry Mandy's comment raised a couple of issues ... On 29/05/2020 7:12 am, Mandy Chung wrote: Hi Harold, On 5/27/20 1:35 PM, Harold Seigel wrote: Incremental webrev: http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/ full webrev: http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/ Class.java 4406 ReflectionData rd = reflectionData(); 4407 ClassDesc[] tmp = rd.permittedSubclasses; 4408 if (tmp != null) { 4409 return tmp; 4410 } 4411 4412 if (isArray() || isPrimitive()) { 4413 rd.permittedSubclasses = new ClassDesc[0]; 4414 return rd.permittedSubclasses; 4415 } This causes an array class or primitive type to create a ReflectionData. It should first check if this is non-sealed class and returns a constant empty array. It can't check if this is a non-sealed class as the isSealed() check calls the above code! But for arrays and primitives which can't be sealed we should just do: 4412 if (isArray() || isPrimitive()) { 4413 return new ClassDesc[0]; 4414 } But this then made me realize that we need to be creating defensive copies of the returned arrays, as happens with other APIs that use ReflectionData. Backing up a bit I complained that: public boolean isSealed() { return permittedSubclasses().length != 0; } is a very inefficient way to answer the question as to whether a class is sealed, so I suggested that the result of permittedSubclasses() be cached. Caching is not without its own issues as we are discovering, and when you add in defensive copies this seems to be trading one inefficiency for another. For nestmates we don't cache getNestMembers() because we don;t think it will be called often - it is there to complete the introspection API of Class rather than being anticipated as used in a regular programmatic sense. I expect the same is true for permittedSubclasses(). Do we expect isSealed() to be used often or is it too just there for completeness? If just for completeness then perhaps a VM query would be a better compromise on the efficiency front? Otherwise I can accept the current implementation of isSealed(), and a non-caching permittedClasses() for this initial implementation of sealed classes. If efficiency turns out to be a problem for isSealed() then we can revisit it then. Thanks, David In fact, ReflectionData caches the derived names and reflected members for performance and also they may be invalidated when the class is redefined. It might be okay to add ReflectionData::permittedSubclasses while `PermittedSubclasses` attribute can't be redefined and getting this attribute is not performance sensitive. For example, the result of `getNestMembers` is not cached in ReflectionData. It may be better not to add it in ReflectionData for modifiable and performance-sensitive data. 4421 tmp = new ClassDesc[subclassNames.length]; 4422 int i = 0; 4423 for (String subclassName : subclassNames) { 4424 try { 4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.')); 4426 } catch (IllegalArgumentException iae) { 4427 throw new InternalError("Invalid type in permitted subclasses information: " + subclassName, iae); 4428 } 4429 } Nit: rename tmp to some other name e.g. descs I read the JVMS but it isn't clear to me that the VM will validate the names in `PermittedSubclasses`attribute are valid class descriptors. I see ConstantPool::is_klass_or_reference check but can't find where it validates the name is
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks for the comments. Here's version 3 of the JDK and VM changes for sealed classes. full webrev: http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/ The new webrev contains just the following three changes: 1. The sealed classes API's in Class.java (permittedSubclasses() and isSealed()) were revised and, in particular, API permittedSubclasses() no longer uses reflection. 2. An unneeded 'if' statement was removed from JVM_GetPermittedSubclasses() (pointed out by David.) 3. VM runtime test files SealedUnnamedModuleIntfTest.java and Permitted.java were changed to add a test case for a non-public permitted subclass and its sealed superclass being in the same module and package. Additionally, two follow on RFE's will be filed. One to add additional VM sealed classes tests and one to improve the implementations of the sealed classes API's in Class.java. Thanks, Harold On 5/28/2020 8:30 PM, David Holmes wrote: Hi Harold, Sorry Mandy's comment raised a couple of issues ... On 29/05/2020 7:12 am, Mandy Chung wrote: Hi Harold, On 5/27/20 1:35 PM, Harold Seigel wrote: Incremental webrev: http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/ full webrev: http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/ Class.java 4406 ReflectionData rd = reflectionData(); 4407 ClassDesc[] tmp = rd.permittedSubclasses; 4408 if (tmp != null) { 4409 return tmp; 4410 } 4411 4412 if (isArray() || isPrimitive()) { 4413 rd.permittedSubclasses = new ClassDesc[0]; 4414 return rd.permittedSubclasses; 4415 } This causes an array class or primitive type to create a ReflectionData. It should first check if this is non-sealed class and returns a constant empty array. It can't check if this is a non-sealed class as the isSealed() check calls the above code! But for arrays and primitives which can't be sealed we should just do: 4412 if (isArray() || isPrimitive()) { 4413 return new ClassDesc[0]; 4414 } But this then made me realize that we need to be creating defensive copies of the returned arrays, as happens with other APIs that use ReflectionData. Backing up a bit I complained that: public boolean isSealed() { return permittedSubclasses().length != 0; } is a very inefficient way to answer the question as to whether a class is sealed, so I suggested that the result of permittedSubclasses() be cached. Caching is not without its own issues as we are discovering, and when you add in defensive copies this seems to be trading one inefficiency for another. For nestmates we don't cache getNestMembers() because we don;t think it will be called often - it is there to complete the introspection API of Class rather than being anticipated as used in a regular programmatic sense. I expect the same is true for permittedSubclasses(). Do we expect isSealed() to be used often or is it too just there for completeness? If just for completeness then perhaps a VM query would be a better compromise on the efficiency front? Otherwise I can accept the current implementation of isSealed(), and a non-caching permittedClasses() for this initial implementation of sealed classes. If efficiency turns out to be a problem for isSealed() then we can revisit it then. Thanks, David In fact, ReflectionData caches the derived names and reflected members for performance and also they may be invalidated when the class is redefined. It might be okay to add ReflectionData::permittedSubclasses while `PermittedSubclasses` attribute can't be redefined and getting this attribute is not performance sensitive. For example, the result of `getNestMembers` is not cached in ReflectionData. It may be better not to add it in ReflectionData for modifiable and performance-sensitive data. 4421 tmp = new ClassDesc[subclassNames.length]; 4422 int i = 0; 4423 for (String subclassName : subclassNames) { 4424 try { 4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.')); 4426 } catch (IllegalArgumentException iae) { 4427 throw new InternalError("Invalid type in permitted subclasses information: " + subclassName, iae); 4428 } 4429 } Nit: rename tmp to some other name e.g. descs I read the JVMS but it isn't clear to me that the VM will validate the names in `PermittedSubclasses`attribute are valid class descriptors. I see ConstantPool::is_klass_or_reference check but can't find where it validates the name is a valid class descriptor - can you point me there? (otherwise, maybe define it to be unspecified?) W.r.t. the APIs. I don't want to delay this review. I see that you renamed the method to new API style as I brought up. OTOH, I expect more discussion is needed. Below is a recent comment from John on this topic [1] One comment, really for the future, regarding the shape of the Java API here: It uses Optional and omits the "get" prefix on accessors. This is the New Style, as opposed to the Classic Style using null (for "empty"
RFR(XS): 8221306: JVMTI spec for FramePop(), MethodExit(), and MethodEnter() could use some cleanup
Please, review a fix for small spec bug: https://bugs.openjdk.java.net/browse/JDK-8221306 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmt-funcs-cleanup.1/src/ Updated JVM TI spec for the FramePop, MethodEntry and MethodExit events: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmt-funcs-cleanup.1/docs/specs/jvmti.html#FramePop Summary: It is a minor spec cleanup for JVM TI events FramePop/MethodEntry/MethodExit: - added small clarification that GetFrameLocation needs to be asked for frame at depth 0 - removed partly unneeded and partly incorrect statements about MethodExit event argument Testing: Manually verified the generated jvmti.html. I think, there is no need to file a CSR for this spec update as it is just minor cleanup. Thanks, Serguei
Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath
Hi David, Also jumping to end. On 5/30/20 06:50, David Holmes wrote: Hi Serguei, Jumping to the end for now ... On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote: Hi David and reviewers, The updated webrev version is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/ This update adds testing that StopThread can return JVMTI_ERROR_INVALID_OBJECT error code. The JVM TI StopThread spec is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread There is a couple of comments below. On 5/29/20 06:18, David Holmes wrote: On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote: On 5/29/20 00:56, serguei.spit...@oracle.com wrote: On 5/29/20 00:42, serguei.spit...@oracle.com wrote: Hi David, Thank you for reviewing this! On 5/28/20 23:57, David Holmes wrote: Hi Serguei, On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote: Hi David, I've updated the CSR and webrev in place. The changes are: - addressed David's suggestion to rephrase StopThread description change - replaced JVMTI_ERROR_INVALID_OBJECT with JVMTI_ERROR_ILLEGAL_ARGUMENT - updated the implementation in jvmtiEnv.cpp to return JVMTI_ERROR_ILLEGAL_ARGUMENT - updated one of the nsk.jvmti StopThread tests to check error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT I'm reposting the links for convenience. Enhancement: https://bugs.openjdk.java.net/browse/JDK-8234882 CSR draft: https://bugs.openjdk.java.net/browse/JDK-8245853 Spec updates are good - thanks. Thank you for the CSR review. Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ src/hotspot/share/prims/jvmtiEnv.cpp The ThreadDeath check is fine but I'm a bit confused about the additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. I can't see how resolve_external_guard can return NULL when not passed in NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT rather than JVMTI_ERROR_NULL_POINTER. And I note JVMTI_ERROR_NULL_POINTER is not even a listed error for StopThread! This part of the change seems unrelated to this issue. I was also surprised with the JVMTI_ERROR_NULL_POINTER and JVMTI_ERROR_INVALID_OBJECT error codes. The JVM TI spec automatic generation adds these two error codes for a jobject parameter. Also, they both are from the Universal Errors section: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error You can find a link to this section at the start of the Error section: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread My understanding (not sure, it is right) is that NULL has to be reported with JVMTI_ERROR_NULL_POINTER and a bad jobject (for instance, a WeakReference with a GC-ed target) has to be reported with JVMTI_ERROR_INVALID_OBJECT. At least, I was not able to construct a test case to get this error code returned. So, I'm puzzled with this. I'll try to find some examples with JVMTI_ERROR_NULL_POINTER errors. Found the explanation. The JDI file: src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java has a fragment that translate the INVALID_OBJECT error to the ObjectCollectedException: RuntimeException toJDIException() { switch (errorCode) { case JDWP.Error.INVALID_OBJECT: return new ObjectCollectedException(); So, the INVALID_OBJECT is for a jobject handle that is referencing a collected object. It means that previous implementation incorrectly returned JVMTI_ERROR_NULL_POINTER error code. I should create and delete local or global ref to construct a test case for this. Interesting that the JDWPException::toJDIException() does not convert the ILLEGAL_ARGUMENT error code to an IllegalArgumentException. I've just added this conversion. Given the definition of JDWP INVALID_OBJECT then obviously JDI converts it to ObjectCollectedException. So reading further in JNI spec: "Weak global references are a special kind of global reference. Unlike normal global references, a weak global reference allows the underlying Java object to be garbage collected. Weak global references may be used in any situation where global or local references are used." So it seems that any function that takes a jobject cxould in fact accept a jweak, in which case JVMTI_ERROR_INVALID_OBJECT is a possibility in all cases. So IIUC JNIHandles::resolve_external_guard can return NULL if a weak reference has been collected. So the new code you propose seems correct. You are right about weak global references. I was able to construct a test case for JVMTI_ERROR_INVALID_OBJECT. The JNI NewGlobalRef and DeleteGlobalRef are used for it. You can find it in the updated webrev version. However, this still is unrelated to the current issue and I do not see other JVM TI doing checks for this case. So this seems to be a much