RFR 8223351 [lworld/lw2] : Primary mirror and nullable mirror for inline type

[Mandy Chung]

On 6/5/19 11:07 AM, Roger Riggs wrote:
> Hi Mandy,
>
> Class.java: 261,  Shouldn't "value" -> "inline"  as does toString().

Good catch.  Fixed.
>
> Line 535:
> I'm a bit confused by the Class.inlineType field.
> It seems to be the secondary mirror (javaClasses.cpp:990) but here is 
> returned as the primary.

989 oop indirect_mirror_oop = create_indirect_type_mirror(k, mirror, CHECK);

This creates a new Class object (secondary mirror) but the inlineType and
indirectType fields are not yet initialized in both primary/secondary mirror.

  990 set_inline_type_mirror(mirror(), mirror());
991 set_indirect_type_mirror(mirror(), indirect_mirror_oop); 1016 
set_inline_type_mirror(indirect_mirror(), mirror());
1017 set_indirect_type_mirror(indirect_mirror(), indirect_mirror()); The 
above initializes the inlineType and indirectType fields to the primary 
mirror and secondary mirror respectively. Note that the first argument 
is the Class object in which the field is being set. Is this clearer?

> Is this backwards?  If it is an inline class, then it is its own primary.

It does set the inline Class's inlineType field to the primary mirror 
which is itself.

> Alternatively, inlineType would/should have been initialized to the 
> primary and a runtime check would not be needed.
>
> 1329:  Can't asPrimaryType() be used without qualification?
> AsPrimaryType should always return the primary and the signers should 
> always be on the primary.
>

Yes until JDK-8225317 is fixed.  asPrimaryType intrinsification needs to 
be updated.  Or I can disable to intrinsification completely.  I don't 
mind doing the cleanup when JDK-8225317 is resolved.

> Everyplace that uses asPrimaryType() would be clearer if it did not 
> double test for inline.
>

Yes, it will be.

> Class.java: 511-516 should not need to be there; the verifier should 
> have already confirmed that.
> Add a comment to remove extra tests.
>

It's taken out.   No need to keep that.

> Line 593:  no need for ?: here; return isIndirectType();
>

Fixed.

> Line 1718:  Use private toTypeName().

Fixed

Mandy

>
> I didn't look too closely at the j.l.invoke parts.
>
> Roger
>
>
> On 06/04/2019 08:27 PM, Mandy Chung wrote:
>> I have updated the patch to add the indirect projection type. Updated 
>> webrev:
>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/lw2/8223351/webrev.01/ 
>>
>>
>> javadoc:
>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/lw2/api/java.base/java/lang/Class.html 
>>
>>
>> Summary of the changes:
>>
>> 1. For an inline class V, V.class is the primary mirror.   The 
>> secondary mirror
>>     is the indirect projection type which is also the nullable 
>> projection type.
>>
>>     Class.forName returns the primary mirror.  The projection types are
>>     used for reflection APIs to find a field of type V? or a method 
>> with signature
>>     with V? (i.e. L-type descriptor).
>>
>> 2. New APIs are added for core reflection to project an inline class 
>> its indirect
>>     or nullable projection type as well as query if a Class object is 
>> indirect or inline
>>     or nullable.
>>
>>     Class::asPrimaryType
>>     Class::asIndirectType
>>     Class::asNullableType
>>
>>     Class::isInlineClass  (was Class::isValue)
>>     Class::isIndirectType
>>     Class::isNullableType
>>
>> 3. Class::getName returns the same name for inline class and the 
>> projection type
>>     as that's the name of the class in the source
>>
>> 4. Class::isAssignableFrom checks properly the subtype relationship
>>     Point <: Point? and Point? <: Object
>>
>> Notes:
>> - I didn't rename ValueKlass::value_mirror as ValueKlass and other 
>> classes will
>>   be renamed together in the future.
>> - I think AccessFlags::set_is_flattenable should be re-examined. 
>> JVM_ACC_FLATTENABLE
>>   no longer set in lworld?
>> - Intrinsification of Class::asPrimaryType will need update 
>> (JDK-8225317).
>>   I have excluded 
>> MyValue1.class.isAssignableFrom(MyValue1.class.asIndirectType())
>>   test case in compiler/valhalla/valuetypes/TestIntrinsifics.java. 
>> I'd need the compiler
>>   expert to help investigating it while I think it might be related 
>> to JDK-8225317.  Hence
>>   I capture that failing case in JDK-8225317.
>> - I took out the unnecessary asPrimaryType call in the tests since 
>> V.class now returns
>>   the primary mirror.  I left several asPrimaryType calls in 
>> TestIntrinsics as they look
>>   like those calls are intended.
>>
>> Mandy
>>
>> On 5/15/19 11:38 AM, Roger Riggs wrote:
>>> Please review Mandy's additions and changes to reflection and 
>>> java.lang.invoke APIs
>>> for inline and nullable types.
>>> The changes go a bit deep because of the support for the Java APIs 
>>> provided by the VM.
>>>
>>> This initial prototype reflects discussions about terminology and 
>>> orthogonality
>>> of concepts for inline vs nullable as described in the comments of 
>>> 8223351.
>>>
>>> Issue:
>>>   https://bugs.openjdk.java.net/browse/JDK-8223351
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~rriggs/webrev-lworld-dev-8223351/
>>>
>>> Thanks for any comments and suggestions, Roger
>>>
>>
>