RFR: JEP 359-Records: hotspot runtime and serviceability code

[Harold Seigel]

Thanks Lois!

Please see comments inline.

On 10/21/2019 2:40 PM, Lois Foltan wrote:
> On 10/18/2019 2:44 PM, Vicente Romero wrote:
>> Hi,
>>
>> Please review the hotspot runtime and serviceability code for JEP 359 
>> (Records).
>>
>> Thanks in advance for the feedback,
>> Vicente
>>
>> PS, Thanks to Harold for the development
>>
>>
>> [1] 
>> http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/
>
> Harold, Vicente,
>
> Overall this looks good!  Some comments:
>
> src/hotspot/share/classfile/classFileParser.cpp
> - line #3226: I mentiond this below that I would like to see the 
> comment from jvmtiClassFileReconstituter.cp ahead of
> ClassFileParser::parse_classfile_record_attribute() so it is easy to 
> follow the expected layout.
Done.
> - line #3275: Ahead of the for loop it would be good to add a comment 
> listing what the expected attributes are for Records.
Done.
> - line #4732: The added check of "!major_gte_14" looks incorrect for 
> final abstract classes.  That isn't relevant to Records, correct?
That's a bug!  Thanks for finding it.  I'll remove the "!major_gte_14".
>
> src/hotspot/share/prims/jvm.cpp
> - line #1737 - #1739: use oopFactory::new_objArrayHandle() instead of 
> breaking this accross 2 lines.

I don't think this should be done because of the CHECK_NULL:

           objArrayOop record_components =
    oopFactory::new_objArray(SystemDictionary::RecordComponent_klass(),
    length, CHECK_NULL);
           objArrayHandle components_h (THREAD, record_components);

> - line #1751: please add a comment that indicates the behavior when 
> the InstanceKlass is not a record.  It seems like an empty array is 
> returned?
Done.
>
> src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp
> - copyright update needed.
> - line #427-438: I like the comment, can you add that ahead of 
> ClassFileParser::parse_classfile_record_attribute().
Done.
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp
> - line #839 - comment "of" should be "if"?
Done.
>
> src/hotspot/share/oops/recordComponent.hpp
> - line #95 - determination of TBD comment needed before committing.
Done.
>
> I still need to review the tests.
Thanks! Harold
>
> Thanks,
> Lois
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191022/f57cdea9/attachment.html>