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

[David Holmes]

On 1/11/2018 12:12 AM, Vicente Romero wrote:
> 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?

The JVM_* API is only for the JDK libraries to use. Many functions, rely 
on the JDK side having established some preconditions and/or invariants.

Cheers,
David

>>
>> 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
>>>
>