RFR: 8230482: Initial support for empty inline classes

Frederic Parain frederic.parain at oracle.com
Thu Sep 5 13:39:02 UTC 2019


Ioi,

Thank you for the review.

Fred


On 9/4/19 4:08 PM, Ioi Lam wrote:
> The new version looks good. Thanks!
> - Ioi
> 
> On 9/4/2019 10:50 AM, Frederic Parain wrote:
>> Ioi,
>>
>> Thank you for reviewing this change.
>>
>> new webrev:
>> http://cr.openjdk.java.net/~fparain/emptyvalue/webrev.02/index.html
>>
>> I’ve added a comment explaining why inline classes need
>> two additional slots.
>>
>> There’s already a test checking that the injected field is not
>> visible via reflection (EmptyValueTest.java:120-127 in the last
>> webrev).
>>
>> I’ve added a few more tests to check that access by reflection to
>> the empty field in WithEmptyField works. However, this path is
>> not optimized yet (i.e. new instances of EmptyValue are created).
>>
>> Regards,
>>
>> Fred
>>
>>> On Sep 4, 2019, at 12:39, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>
>>> Hi Fred,
>>>
>>> The changes look good to me.
>>>
>>> 1549   const int total_fields = length + num_injected + 
>>> (is_value_type ? 2 : 0);
>>>
>>> Maybe add a comment what the "2" means?
>>>
>>> Also, I think it will be good to add a Java reflection test to query 
>>> the fields in
>>> EmptyValue, to make sure zero fields are returned.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>> On 9/4/19 9:28 AM, Frederic Parain wrote:
>>>> Remi, Ioi,
>>>>
>>>> Here’s a new webrev with the new tests added:
>>>>
>>>> http://cr.openjdk.java.net/~fparain/emptyvalue/webrev.01/
>>>>
>>>> On a 64bits platform with compressed oops, the layout of
>>>> the WithEmptyField class is:
>>>>
>>>> Layout of class 
>>>> runtime/valhalla/valuetypes/EmptyValueTest$WithEmptyField
>>>> | offset | kind | size | alignment | signature | name |
>>>> Instance fields:
>>>>    0 RESERVED 12
>>>>    12 INHERITED 4
>>>>    16 FLATTENED 1 1 
>>>> Qruntime/valhalla/valuetypes/EmptyValueTest$EmptyValue; empty
>>>>    17 EMPTY 3
>>>>    20 REGULAR 4 4 Ljava/lang/Object; o
>>>>
>>>> The inherited field at offset 12 being the int field from the super 
>>>> class.
>>>>
>>>> Thank you,
>>>>
>>>> Fred
>>>>
>>>>
>>>>> On Sep 3, 2019, at 11:41, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>>
>>>>> Maybe we should also have some System.arraycopy() tests?
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>> On 9/3/19 8:29 AM, Remi Forax wrote:
>>>>>> Hi Frederic,
>>>>>> i think you can add a test where an empty inline object is used as 
>>>>>> field in the middle of several fields
>>>>>> (perhaps using inheritance to be sure of the layout ?).
>>>>>>
>>>>>> Rémi
>>>>>>
>>>>>> ----- Mail original -----
>>>>>>> De: "Frederic Parain" <frederic.parain at oracle.com>
>>>>>>> À: "valhalla-dev" <valhalla-dev at openjdk.java.net>
>>>>>>> Envoyé: Mardi 3 Septembre 2019 16:24:08
>>>>>>> Objet: RFR: 8230482: Initial support for empty inline classes
>>>>>>> Greetings,
>>>>>>>
>>>>>>> Please review this code that adds initial support for empty 
>>>>>>> inline classes.
>>>>>>>
>>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8230482
>>>>>>> Webrev: 
>>>>>>> http://cr.openjdk.java.net/~fparain/emptyvalue/webrev.00/index.html
>>>>>>>
>>>>>>> Thank you,
>>>>>>>
>>>>>>> Fred
> 


More information about the valhalla-dev mailing list