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

[David Holmes]

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.

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.

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.)

---

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.

+     jio_snprintf(sig, sig_len, "()%s", type->as_C_string());

You should use the symbolic constants for the '(' and ')' characters.

---

src/hotspot/share/oops/instanceKlass.cpp

Nit:

3549       if (component) {

should test != NULL

---

  test/jdk/java/lang/instrument/RedefineRecordAttr

Nice reuse of the nestmate testing pattern :)

---

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