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

[Vicente Romero]

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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: record.hotspot.v3.patch
Type: text/x-patch
Size: 35637 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20181031/63781312/record.hotspot.v3-0001.patch>