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