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

[Harold Seigel]

The changes were pushed to the records branch of the amber repo: 
http://hg.openjdk.java.net/amber/amber

Harold

On 10/25/2019 9:23 AM, David Holmes wrote:
> On 25/10/2019 9:49 pm, Harold Seigel wrote:
>> Thanks David!
>>
>> I should have mentioned that I had pushed Serguei's jvmti.xml and 
>> TestRecordAttr*.java changes earlier.
>
> Pushed under what bug id to where? This JEP is still only a Candidate.
>
> David
>
>> Harold
>>
>> On 10/24/2019 8:40 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 25/10/2019 3:50 am, 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 
>>>>
>>>
>>> That incremental webrev looks fine.
>>>
>>> However you didn't include Serguei's earlier change request
>>>
>>> [Harold]>> I'll fix line 7789 in jvmti.xml before the change gets 
>>> pushed.
>>>
>>> so just wanted to remind you on that.
>>>
>>> Thanks,
>>> David
>>>
>>>> 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/ 
>>>>>>