RFR: JDK-8225056 VM support for sealed classes

Mon Jun 1 13:07:04 UTC 2020

Hi David,

Thanks for reviewing the latest changes.

I'll create the follow on RFE's once the sealed classes code is in mainline.

Harold

On 5/31/2020 9:34 PM, David Holmes wrote:
> 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<T> 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" results) and a "get" prefix 
>>>>> ("getComponentType") to get a related type. We may choose to to 
>>>>> use the New Style for new reflection API points, and if so let's 
>>>>> not choose N different New Styles, but one New Style. Personally I 
>>>>> like removing "get"; I accept Optional instead of null; and I also 
>>>>> suggest that arrays (if any) be replaced by (immutable) Lists in 
>>>>> the New Style
>>>>
>>>> There are a few existing Class APIs that use the new API style:
>>>> Class<?> arrayClass();
>>>> Optional<ClassDesc> describeConstable();
>>>> String descriptorString();
>>>>
>>>> This will set up a precedence of the new API style in this class.  
>>>> Should this new permittedSubclasses method return a List instead of 
>>>> an array?  It's okay with me if you prefer to revert back to the 
>>>> old API style and follow this up after integration.
>>>>
>>>> 4442 * Returns true if this {@linkplain Class} is sealed.
>>>> 4443 *
>>>> 4444 * @return returns true if this class is sealed
>>>>
>>>> NIt: {@code true} instead of true -  consistent with the style this 
>>>> class uses (in most methods)
>>>>
>>>> test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java 
>>>>
>>>>
>>>> Nit: s/sealed_classes/sealedClasses/
>>>> - the test directory/file naming convention use camel case or java 
>>>> variable name convention.
>>>>
>>>> Thanks
>>>> [1] https://github.com/openjdk/valhalla/pull/53#issuecomment-633116043