RFR: 8230482: Initial support for empty inline classes

[Ioi Lam]

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