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

[David Holmes]

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.

Cheers,
David

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
>