[amber-record] feedback on a Record class attribute implementation

[Vicente Romero]

Hi David,

On 10/30/18 11:59 PM, David Holmes wrote:
> Hi Vicente,
>
> Thanks for the responses.
>
> For the JVM_* functions I find it easier to do the primitive/array 
> class checks on the Java side and just assume at the VM level (with 
> suitable assertion) that you're only working with instance classes. 
> But I don't think there's any real consistency in how the labour is 
> divided.

while I was doing this change I was thinking if the check should be in 
the VM side just in case it is invoked from a language different to 
Java. I think that the VM side should be defensive just in case. What do 
you think?

>
> Cheers,
> David

Thanks,
Vicente

>
> On 31/10/2018 1:21 PM, Vicente Romero wrote:
>> Hi David,
>>
>> Thanks for your comments, I have attached another iteration of the 
>> patch. In the mean time I have added another JNI method which is also 
>> included in this iteration. Some additional comments below.
>>
>> On 10/30/18 9:57 PM, David Holmes wrote:
>>> Hi Vicente,
>>>
>>> On 31/10/2018 10:59 AM, Vicente Romero wrote:
>>>> Hi all,
>>>>
>>>> I have sent the patch to the hotspot list as this patch is touching 
>>>> some native code and I would like to have some feedback from the 
>>>> runtime wizards, please let me know if there is a better list for 
>>>> that. This 
>>>
>>> hotspot-runtime-dev :)
>>
>> removing hotspot-dev and adding hotspot-runtime-dev :)
>>
>>>
>>> A couple of minor comments:
>>>
>>> src/hotspot/share/classfile/classFileParser.cpp
>>>
>>> +  // Set nest members attribute to default sentinel
>>> +  _record_params = Universe::the_empty_short_array();
>>>
>>> Comment needs updating.
>>
>> right I updated it
>>
>>>
>>> -        } else {
>>> -          // Unknown attribute
>>> -          cfs->skip_u1(attribute_length, CHECK);
>>> +        } else if (tag == vmSymbols::tag_record()) {
>>> ...
>>>
>>> You need to insert the new check before the existing else clause but 
>>> need to keep the check for an unknown attribute in the given 
>>> classfile version. Presumably this will eventually be processed only 
>>> for some future classfile version.
>>
>> good catch!
>>
>>>
>>> ---
>>>
>>> src/hotspot/share/oops/instanceKlass.hpp
>>>
>>> +  u2              _record_params_count;
>>>
>>> Do you need a separate count when you can query the array length?
>>
>> yes you need this field because the array is declared as: Array<u2>* 
>> _record_params and the actual fields are accessed using offsets. I 
>> just replicated what is currently being done for fields just to be 
>> consistent with the existing code.
>>
>>>
>>> Cheers,
>>> David
>>
>> Thanks!
>> Vicente
>>
>>>
>>>> patch has been pushed the the record branch in amber project [1]. 
>>>> The records project is about, well adding data classes to the 
>>>> language so that this declaration:
>>>>
>>>> record Record(int i);
>>>>
>>>> gets lowered to:
>>>>
>>>> class Record {
>>>>      final int i;
>>>>
>>>>     // automatically generated equals, getters, hashCode, toString 
>>>> and more
>>>> }
>>>>
>>>> apart from the usual information generated for the lowered version, 
>>>> the javac compiler is generating this new attribute in the class file:
>>>>
>>>>          Record {
>>>>              u2 name_index;
>>>>              u4 length;
>>>>              u2 num_record_params;
>>>>              {
>>>>                  u2 param_name_idx;  // [1]
>>>>                  u2 param_flag;
>>>>                  u2 param_desc;
>>>>                  u2 param_signature;
>>>>              } record_params[num_record_params];
>>>>          }
>>>>
>>>> which have a lot in common with the fields information but we don't 
>>>> want to depend on the order of the fields etc. The attached patch 
>>>> provides for parsing this attribute, plus additional helper 
>>>> classes, plus all the pipes needed. As a way to provide a way for 
>>>> users to peek the information contained in the Record attribute, I 
>>>> have added a method to java.lang.Class, Class::getRecordParameters. 
>>>> In the background I'm using JNI to extract the information from the 
>>>> related InstanceKlass in the native world. Method 
>>>> java.lang.Class::getRecordParameters just returns an array of 
>>>> fields but only those that have being defined in the header of the 
>>>> record. For example if the record would have been defined as:
>>>>
>>>> record Record(int i) {
>>>>      static final int j = 0;  //no instance fields can be defined 
>>>> in records
>>>> }
>>>>
>>>> then an invocation of java.lang.Class::getRecordParameters will 
>>>> return only field `i` ignoring the static field `j`
>>>>
>>>> TIA,
>>>> Vicente
>>>>
>>>> [1] http://hg.openjdk.java.net/amber/amber
>>