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

Vicente Romero vicente.romero at oracle.com
Wed Oct 31 03:21:43 UTC 2018


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.v2.patch
Type: text/x-patch
Size: 36119 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20181030/067c7e85/record.hotspot.v2-0001.patch>


More information about the hotspot-runtime-dev mailing list