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

Thu Jun 6 20:13:52 UTC 2019

Hi Mandy,

Why were you able to delete these lines from javaClasses.cpp?

    1010 if (k->is_array_klass()) {
    1011 assert(component_mirror(mirror()) != NULL, "must have a mirror");
    1012 set_component_mirror(value_mirror(), component_mirror(mirror()));
    1013 }

Does the component mirror get set elsewhere?

Thanks, Harold

On 6/6/2019 4:03 PM, Karen Kinnear wrote:
> Mandy caught that I accidentally reviewed the .00 webrev not .01
>
> webrev.01 hotspot source and test files look good - and fits your chart better.
>
> Thank you for making the vm changes!
>
> thanks for catching that,
> Karen
>
>
>> On Jun 6, 2019, at 3:24 PM, Karen Kinnear <karen.kinnear at oracle.com> wrote:
>>
>> Mandy,
>>
>> I reviewed the hotspot changes, source and tests and double-checked my only question with Tobias.
>>
>> They look good. In particular the logic of setting the mirrors looks right.
>>
>> Minor comment:
>> Class.java line 561: the final “and” in this line should be an “or”
>>
>> JVM_ACC_FLATTENABLE: Agreed we need to revisit this.
>>
>> thanks,
>> Karen
>>
>>> On Jun 5, 2019, at 2:36 PM, Mandy Chung <mandy.chung at oracle.com> wrote:
>>>
>>>
>>>
>>> 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
>>>>>>