[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