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

[Vicente Romero]

On 10/31/18 7:23 PM, David Holmes wrote:
> On 1/11/2018 9:08 AM, Vicente Romero wrote:
>> On 10/31/18 6:22 PM, David Holmes wrote:
>>> 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.
>>
>> I see, I have attached another iteration of the patch that moves the 
>> checks to the java side
>
> There was no need to do that. As I said there is no clear division of 
> labor here. It's just often easier to write these checks and produce 
> empty arrays etc, in Java than C++.

yep and that made sense to me, anyways it is done already :)

>
> Cheers,
> David

Thanks for your comments,
Vicente
>
>>
>>>
>>> Cheers,
>>> David
>>
>> Thanks!
>> Vicente
>>
>>>
>>>>>
>>>>> 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
>>>>>>
>>>>
>>