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

Mandy Chung mandy.chung at oracle.com
Thu Jun 6 20:28:56 UTC 2019


When a component type is an inline type, the Class object or the 
component type
should have been created and initialized, i.e. its primary and indirect 
type mirrors
already set up .  So they are not needed.

line 926-946 gets the component type of an array class and line 940-941 are
the assertion.

Mandy

On 6/6/19 1:13 PM, Harold Seigel wrote:
> 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
>>>>>>>



More information about the valhalla-dev mailing list