[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JEP 359-Records: hotspot runtime and serviceability code
From:       David Holmes <david.holmes () oracle ! com>
Date:       2019-10-27 22:34:43
Message-ID: 8167dced-416a-9c70-a88d-76881ce90aba () oracle ! com
[Download RAW message or body]

On 25/10/2019 11:32 pm, Harold Seigel wrote:
> The changes were pushed to the records branch of the amber repo: 
> http://hg.openjdk.java.net/amber/amber

Okay. I thought we were doing a code review ready for this to go into 
mainline.

David
-----

> 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/ 
>>>>>>>
[prev in list] [next in list] [prev in thread] [next in thread]