Re: RFR: JEP 359-Records: hotspot runtime and serviceability code
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/
Re: RFR: JEP 359-Records: hotspot runtime and serviceability code
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
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/
Re: DumpJFR tool
On 2019-10-24 16:52, Yasumasa Suenaga wrote: On 2019/10/24 22:50, Erik Gahlin wrote: Hi Yasumasa, The DumpJFR tool relied on SA, which was too much of a burden to maintain (slowing down the development of new JFR features). I think tool was removed in Oracle JDK 9, at least it was completely broken at that time. Functionality has been added that allows JFR to dump an emergency dump if the JVM crashes. This reduces the need for a separate tool. SA is powerful tool for hung up and for postmortem analysis. For example, SA is only way to gather any information even if Attach Listener does not respond. There are always trade-offs, and with the crash dump support in place, we came to the conclusion that the cost of maintaining the tool and SA support outweighed the benefits. JFR emergency dump is not enough. I commented in JDK-8213435, crash which is caused by -XX:+CrashOnOutOfMemoryError does not contain recently old object sampling. Duplicating logic in the SA tool to calculate shortest path GC, information about root objects etc. would be a much larger effort to implement and maintain than fixing JDK-8213435. In addition, there are some cases not to be called crash handler (e.g. native stack overflow). The Event Streaming JEP, targeted for JDK 14, should also help out here, as it ensures the latest file in the disk repository, previously known as the .part file, can always read. With a default flush interval of 1 second, data will be available very close to the point of the crash. Is flight record flushed to the disk without any programming for Event Streaming? I guess it is just about Event Stream API. I hope to get flight record with some configuration - not any programming. Streaming will be always on. No programming or configuration will be needed. You can just open the latest file in JMC. Best regards, Erik Thanks, Yasumasa Best regards, Erik Hi all, DumpJFR tool is introduced in JDK 8u60 [1], however it is not available on OpenJDK (JDK 11 or later). Why it is not open sourced? I think DumpJFR is very useful not only for postmortem analysis but also analyzing process hung up. If DumpJFR is available on OpenJDK, I believe JFR will be more useful for troubleshooting. Thanks, Yasumasa [1] https://www.oracle.com/technetwork/java/javase/8u60-relnotes-2620227.html
Re: RFR: JDK-8232973: Potential infinite loop in macOS hotspot agent
Hi Simon, +1 Thanks, Serguei On 10/24/19 08:55, Chris Plummer wrote: Hi Simon, The changes look good. thanks, Chris On 10/24/19 7:16 AM, Simon Tooke wrote: Hello, While reviewing uses of strtok() with an eye to moving to strtok_r(), I came accross an inifinite loop in the macOS agent code, but one that has probably never been executed. In the interests of not having even potential loops, I've file a bug and have a PR to submit. My patch removes the inifinite loop and switches to strtok_r(). The switch to the reentrant version is not required in this use case but I include it so that this code doesn't show up on future scans for strtok() usage. Bug: https://bugs.openjdk.java.net/browse/JDK-8232973 Webrev: http://cr.openjdk.java.net/~stooke/webrevs/jdk-8232973-jdk/00/ Are there any concerns? Thanks, -Simon
Re: RFR: JDK-8232973: Potential infinite loop in macOS hotspot agent
Looks good.
There is also a possible buffer overrun some lines above:
357 char* posbin = strstr(execname, "/bin/java");
358 if (posbin != NULL) {
359 memcpy(filepath, execname, posbin - execname);// not include
trailing '/'
360 filepath[posbin - execname] = '\0';
depending on the execname length.
Also, the strstr() is probably wrong since it also fires for paths which
have "/bin/java" somewhere in the middle, however unlikely that my be.
I am fine with your change as it is though.
Cheers, Thomas
On Thu, Oct 24, 2019 at 4:16 PM Simon Tooke wrote:
> Hello,
>
> While reviewing uses of strtok() with an eye to moving to strtok_r(), I
> came accross an inifinite loop in the macOS agent code, but one that has
> probably never been executed. In the interests of not having even
> potential loops, I've file a bug and have a PR to submit. My patch
> removes the inifinite loop and switches to strtok_r(). The switch to
> the reentrant version is not required in this use case but I include it
> so that this code doesn't show up on future scans for strtok() usage.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8232973
>
> Webrev: http://cr.openjdk.java.net/~stooke/webrevs/jdk-8232973-jdk/00/
>
> Are there any concerns?
>
> Thanks,
>
> -Simon
>
>
>
>
Re: RFR: JDK-8232973: Potential infinite loop in macOS hotspot agent
Hi Simon, The changes look good. thanks, Chris On 10/24/19 7:16 AM, Simon Tooke wrote: Hello, While reviewing uses of strtok() with an eye to moving to strtok_r(), I came accross an inifinite loop in the macOS agent code, but one that has probably never been executed. In the interests of not having even potential loops, I've file a bug and have a PR to submit. My patch removes the inifinite loop and switches to strtok_r(). The switch to the reentrant version is not required in this use case but I include it so that this code doesn't show up on future scans for strtok() usage. Bug: https://bugs.openjdk.java.net/browse/JDK-8232973 Webrev: http://cr.openjdk.java.net/~stooke/webrevs/jdk-8232973-jdk/00/ Are there any concerns? Thanks, -Simon
Re: DumpJFR tool
On 2019/10/24 22:50, Erik Gahlin wrote: Hi Yasumasa, The DumpJFR tool relied on SA, which was too much of a burden to maintain (slowing down the development of new JFR features). I think tool was removed in Oracle JDK 9, at least it was completely broken at that time. Functionality has been added that allows JFR to dump an emergency dump if the JVM crashes. This reduces the need for a separate tool. SA is powerful tool for hung up and for postmortem analysis. For example, SA is only way to gather any information even if Attach Listener does not respond. JFR emergency dump is not enough. I commented in JDK-8213435, crash which is caused by -XX:+CrashOnOutOfMemoryError does not contain recently old object sampling. In addition, there are some cases not to be called crash handler (e.g. native stack overflow). The Event Streaming JEP, targeted for JDK 14, should also help out here, as it ensures the latest file in the disk repository, previously known as the .part file, can always read. With a default flush interval of 1 second, data will be available very close to the point of the crash. Is flight record flushed to the disk without any programming for Event Streaming? I guess it is just about Event Stream API. I hope to get flight record with some configuration - not any programming. Thanks, Yasumasa Best regards, Erik Hi all, DumpJFR tool is introduced in JDK 8u60 [1], however it is not available on OpenJDK (JDK 11 or later). Why it is not open sourced? I think DumpJFR is very useful not only for postmortem analysis but also analyzing process hung up. If DumpJFR is available on OpenJDK, I believe JFR will be more useful for troubleshooting. Thanks, Yasumasa [1] https://www.oracle.com/technetwork/java/javase/8u60-relnotes-2620227.html
RFR: JDK-8232973: Potential infinite loop in macOS hotspot agent
Hello, While reviewing uses of strtok() with an eye to moving to strtok_r(), I came accross an inifinite loop in the macOS agent code, but one that has probably never been executed. In the interests of not having even potential loops, I've file a bug and have a PR to submit. My patch removes the inifinite loop and switches to strtok_r(). The switch to the reentrant version is not required in this use case but I include it so that this code doesn't show up on future scans for strtok() usage. Bug: https://bugs.openjdk.java.net/browse/JDK-8232973 Webrev: http://cr.openjdk.java.net/~stooke/webrevs/jdk-8232973-jdk/00/ Are there any concerns? Thanks, -Simon
Re: DumpJFR tool
Hi Yasumasa, The DumpJFR tool relied on SA, which was too much of a burden to maintain (slowing down the development of new JFR features). I think tool was removed in Oracle JDK 9, at least it was completely broken at that time. Functionality has been added that allows JFR to dump an emergency dump if the JVM crashes. This reduces the need for a separate tool. The Event Streaming JEP, targeted for JDK 14, should also help out here, as it ensures the latest file in the disk repository, previously known as the .part file, can always read. With a default flush interval of 1 second, data will be available very close to the point of the crash. Best regards, Erik Hi all, DumpJFR tool is introduced in JDK 8u60 [1], however it is not available on OpenJDK (JDK 11 or later). Why it is not open sourced? I think DumpJFR is very useful not only for postmortem analysis but also analyzing process hung up. If DumpJFR is available on OpenJDK, I believe JFR will be more useful for troubleshooting. Thanks, Yasumasa [1] https://www.oracle.com/technetwork/java/javase/8u60-relnotes-2620227.html
Re: RFR: JEP 359-Records: hotspot runtime and serviceability code
Thanks Serguei!
I will make the changes that you suggest.
Harold
On 10/24/2019 2:01 AM, serguei.spit...@oracle.com wrote:
Hi Vicente and Harold,
The fix looks good to me.
Nice set of tests!
I have a couple of nits besides what other reviewers already commented.
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/test/jdk/java/lang/instrument/RedefineRecordAttr/HostBA/redef/Host.java.html
29 public Host(int A, long B, char C) {
30 this.A = A;
31 this.B = B;
32 }
The lines 30 and 31 needs to be swapped to follow the other such
variants style.
For instance the version HostBAC:
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/test/jdk/java/lang/instrument/RedefineRecordAttr/HostBAC/redef/Host.java.html
has constructor:
29 public Host(int A, long B, char C) {
30 this.B = B;
31 this.A = A;
32 this.C = C;
33 }
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/test/jdk/java/lang/instrument/RedefineRecordAttr/TestRecordAttr.java.html
50 The basic test class is call Host and we have variants that have zero or
more
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/test/jdk/java/lang/instrument/RedefineRecordAttrGenericSig/TestRecordAttrGenericSig.java.html
46 The basic test class is call Host and we have variants that have record
components
It seems there is a typo in the comments above: 'is call' => 'is called'.
Maybe, I did not get it correctly.
Thanks,
Serguei
On 10/18/19 11:44, Vicente Romero wrote:
Hi,
Please review the hotspot runtime and serviceability code for JEP 359
(Records).
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/
DumpJFR tool
Hi all, DumpJFR tool is introduced in JDK 8u60 [1], however it is not available on OpenJDK (JDK 11 or later). Why it is not open sourced? I think DumpJFR is very useful not only for postmortem analysis but also analyzing process hung up. If DumpJFR is available on OpenJDK, I believe JFR will be more useful for troubleshooting. Thanks, Yasumasa [1] https://www.oracle.com/technetwork/java/javase/8u60-relnotes-2620227.html
Re: RFR: JEP 359-Records: hotspot runtime and serviceability code
Hi Vicente and Harold,
The fix looks good to me.
Nice set of tests!
I have a couple of nits besides what other reviewers already
commented.
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/test/jdk/java/lang/instrument/RedefineRecordAttr/HostBA/redef/Host.java.html
29 public Host(int A, long B, char C) {
30 this.A = A;
31 this.B = B;
32 }
The lines 30 and 31 needs to be swapped to follow the other such
variants style.
For instance the version HostBAC:
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/test/jdk/java/lang/instrument/RedefineRecordAttr/HostBAC/redef/Host.java.html
has constructor:
29 public Host(int A, long B, char C) {
30 this.B = B;
31 this.A = A;
32 this.C = C;
33 }
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/test/jdk/java/lang/instrument/RedefineRecordAttr/TestRecordAttr.java.html
50 The basic test class is call Host and we have variants that have zero or more
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/test/jdk/java/lang/instrument/RedefineRecordAttrGenericSig/TestRecordAttrGenericSig.java.html
46 The basic test class is call Host and we have variants that have record components
It seems there is a typo in the comments above: 'is call' =>
'is called'.
Maybe, I did not get it correctly.
Thanks,
Serguei
On 10/18/19 11:44, Vicente Romero wrote:
Hi,
Please review the hotspot runtime and serviceability code for JEP
359 (Records).
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/
