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

[Harold Seigel]

Thanks Serguei!

Harold

On 10/25/2019 1:21 PM, serguei.spitsyn at oracle.com wrote:
> Hi Harold,
>
> The update looks good.
>
> Thanks,
> Serguei
>
> On 10/24/19 10:50, Harold Seigel wrote:
>> Hi David,
>>
>> Thanks for reviewing this!
>>
>> Here's an updated webrev containing the below changes (and one change 
>> requested by Serguei).  Could you take a look?
>>
>> http://cr.openjdk.java.net/~hseigel/records.review.rt.01/webrev/index.html 
>>
>>
>> Please also see in-line comments.
>>
>> On 10/23/2019 7:16 PM, David Holmes wrote:
>>> Hi Vicente, (and Harold!)
>>>
>>> On 19/10/2019 4:44 am, Vicente Romero wrote:
>>>> Hi,
>>>>
>>>> Please review the hotspot runtime and serviceability code for JEP 
>>>> 359 (Records).
>>>
>>> I've looked at all the code, though not in great detail i.e I have 
>>> not validated the code changes against the proposed specification. 
>>> Support for records seems mostly "mechanical" and the patterns you 
>>> have used look appropriate. A couple of comments below.
>>>
>>> src/hotspot/share/classfile/classFileParser.cpp
>>>
>>> You added the check
>>>
>>> 3704         } else if (tag == vmSymbols::tag_record()) {
>>>
>>> inside the block
>>>
>>> 3671       } else if (_major_version >= JAVA_11_VERSION) {
>>>
>>> but I would have expected to see a new block created
>>>
>>>        } else if (_major_version >= JAVA_14_VERSION) {
>>>
>>> to hold this code.
>> Done.
>>>
>>> Style nit:
>>>
>>> 3773     const unsigned int calculated_attr_length = 
>>> parse_classfile_record_attribute(
>>> 3774                             cfs,
>>> 3775                             cp,
>>> 3776                             record_attribute_start,
>>> 3777                             CHECK);
>>>
>>> The style in this file is align the args on the = character.
>> Done.
>>>
>>> 4928   if ((is_abstract && is_final && !major_gte_14) ||
>>>
>>> As Lois mentioned already this change seems incorrect in general - 
>>> is it related to sealed types perhaps? (Even then it should be 
>>> tightened to actually check for a sealed type and not just allow 
>>> arbitrary abstract+final classes.)
>> The '!major_gte_14' check was initially for sealed types, but is 
>> wrong.  It's already been removed.
>>>
>>> ---
>>>
>>> src/hotspot/share/classfile/javaClasses.cpp
>>>
>>> +   if (ik->should_be_initialized()) {
>>> +     ik->initialize(CHECK_0);
>>> +   }
>>>
>>> Unless the call to should_be_initialized is an inline method (which 
>>> it isn't) then we may as well just call initialize unconditionally 
>>> as the first thing it will do is check should_be_initialized.
>> Done.
>>>
>>> +     jio_snprintf(sig, sig_len, "()%s", type->as_C_string());
>>>
>>> You should use the symbolic constants for the '(' and ')' characters.
>> Done.
>>>
>>> ---
>>>
>>> src/hotspot/share/oops/instanceKlass.cpp
>>>
>>> Nit:
>>>
>>> 3549       if (component) {
>>>
>>> should test != NULL
>> Done.
>>>
>>> ---
>>>
>>>  test/jdk/java/lang/instrument/RedefineRecordAttr
>>>
>>> Nice reuse of the nestmate testing pattern :)
>>
>> Yes, that nestmate test was very helpful!
>>
>> Thanks, Harold
>>
>>>
>>> ---
>>>
>>> Thanks,
>>> David
>>>
>>>> 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/ 
>>>>
>