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

[David Holmes]

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