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

[David Holmes]

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