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

[Vicente Romero]

On 10/31/18 7:30 PM, David Holmes wrote:
> PS. But if you do the checks on the Java side you should document the 
> expectations on the incoming class. E.g. see JVM_GetNestHost/Members

I see, I will document the methods

>
> David

Vicente

>
> On 1/11/2018 9:23 AM, 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++.
>>
>> Cheers,
>> David
>>
>>>
>>>>
>>>> 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
>>>>>>>
>>>>>
>>>