[amber-record] feedback on a Record class attribute implementation
David Holmes
david.holmes at oracle.com
Wed Oct 31 22:22:08 UTC 2018
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.
Cheers,
David
>>
>> 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
>>>
>
More information about the amber-dev
mailing list