RFR: JDK-8225056 VM support for sealed classes

[David Holmes]

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