[amber-record] feedback on a Record class attribute implementation (original) (raw)

Vicente Romero vicente.romero at oracle.com
Wed Oct 31 14:12:52 UTC 2018

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?

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 +  recordparams = Universe::theemptyshortarray(); Comment needs updating. right I updated it -        } else { -          // Unknown attribute -          cfs->skipu1(attributelength, CHECK); +        } else if (tag == vmSymbols::tagrecord()) { ... 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              recordparamscount; 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* recordparams 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 nameindex; u4 length; u2 numrecordparams; { u2 paramnameidx;  // [1] u2 paramflag; u2 paramdesc; u2 paramsignature; } recordparams[numrecordparams]; } 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 hotspot-runtime-dev mailing list