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

[David Holmes]

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/ 
>>>>>