Integrated: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
On Tue, 15 Sep 2020 16:43:01 GMT, Daniil Titov wrote: > The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes() > when the current thread is in the middle > of redefining the same class. The change is based on the fix [1] suggested in > the Jira issue [2] comment. > [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/ > [2] https://bugs.openjdk.java.net/browse/JDK-8241390 > > Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass. This pull request has now been integrated. Changeset: f800af97 Author:Daniil Titov URL: https://git.openjdk.java.net/jdk/commit/f800af97 Stats: 179 lines in 4 files changed: 0 ins; 170 del; 9 mod 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes() Reviewed-by: coleenp, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/190
Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes() [v3]
> The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes() > when the current thread is in the middle > of redefining the same class. The change is based on the fix [1] suggested in > the Jira issue [2] comment. > [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/ > [2] https://bugs.openjdk.java.net/browse/JDK-8241390 > > Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass. Daniil Titov has updated the pull request incrementally with one additional commit since the last revision: Corrected test summary - Changes: - all: https://git.openjdk.java.net/jdk/pull/190/files - new: https://git.openjdk.java.net/jdk/pull/190/files/3502f018..a2609929 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=190=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=190=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/190.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/190/head:pull/190 PR: https://git.openjdk.java.net/jdk/pull/190
Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes() [v2]
> The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes() > when the current thread is in the middle > of redefining the same class. The change is based on the fix [1] suggested in > the Jira issue [2] comment. > [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/ > [2] https://bugs.openjdk.java.net/browse/JDK-8241390 > > Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass. Daniil Titov has updated the pull request incrementally with one additional commit since the last revision: Changed test to not use shell scripts Additional changes: - Replaced 'cls' identifier with 'redef_classes' - Added a comment that the same class can be pushed to the list multiple times. Testing: tier1-tier3 tests pass. - Changes: - all: https://git.openjdk.java.net/jdk/pull/190/files - new: https://git.openjdk.java.net/jdk/pull/190/files/6e6b1b81..3502f018 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=190=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=190=00-01 Stats: 275 lines in 6 files changed: 119 ins; 142 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/190.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/190/head:pull/190 PR: https://git.openjdk.java.net/jdk/pull/190
RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes() when the current thread is in the middle of redefining the same class. The change is based on the fix [1] suggested in the Jira issue [2] comment. [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/ [2] https://bugs.openjdk.java.net/browse/JDK-8241390 Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass. - Commit messages: - 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes() Changes: https://git.openjdk.java.net/jdk/pull/190/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=190=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8241390 Stats: 202 lines in 6 files changed: 193 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/190.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/190/head:pull/190 PR: https://git.openjdk.java.net/jdk/pull/190
Integrated: 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType
On Fri, 11 Sep 2020 01:08:53 GMT, Daniil Titov wrote: > The change fixes the regression introduced by > https://bugs.openjdk.java.net/browse/JDK-8241080. > > Method validateAssignment() in com.sun.tools.jdi.ObjectReferenceImpl now > always retrieves the reference type and that > requires one more JDWP packet if the value is not cached yet. Before > https://bugs.openjdk.java.net/browse/JDK-8241080 > this happened for arrays only. Testing: tier1-tier3 tests passes. This pull request has now been integrated. Changeset: 65d6c101 Author:Daniil Titov URL: https://git.openjdk.java.net/jdk/commit/65d6c101 Stats: 6 lines in 1 file changed: 1 ins; 3 del; 2 mod 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType Reviewed-by: cjplummer, amenkov - PR: https://git.openjdk.java.net/jdk/pull/124
Re: RFR: 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType
On Fri, 11 Sep 2020 04:00:35 GMT, Chris Plummer wrote: >> The change fixes the regression introduced by >> https://bugs.openjdk.java.net/browse/JDK-8241080. >> >> Method validateAssignment() in com.sun.tools.jdi.ObjectReferenceImpl now >> always retrieves the reference type and that >> requires one more JDWP packet if the value is not cached yet. Before >> https://bugs.openjdk.java.net/browse/JDK-8241080 >> this happened for arrays only. Testing: tier1-tier3 tests passes. > > The changes look good to me. Can I ask how you noticed the extra unnecessary > JDWP packet? Thanks @plummercj for the review. The issue with an extra JDWP packet was reported in serviceability-dev mail list : https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-September/032960.html . - PR: https://git.openjdk.java.net/jdk/pull/124
RFR: 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType
The change fixes the regression introduced by https://bugs.openjdk.java.net/browse/JDK-8241080. Method validateAssignment() in com.sun.tools.jdi.ObjectReferenceImpl now always retrieves the reference type and that requires one more JDWP packet if the value is not cached yet. Before https://bugs.openjdk.java.net/browse/JDK-8241080 this happened for arrays only. Testing: tier1-tier3 tests passes. - Commit messages: - 8252933: com.sun.tools.jdi.ObjectReferenceImpl#validateAssignment always requests referenceType Changes: https://git.openjdk.java.net/jdk/pull/124/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=124=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8252933 Stats: 6 lines in 1 file changed: 3 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/124.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/124/head:pull/124 PR: https://git.openjdk.java.net/jdk/pull/124
Re: 8241080: Consolidate signature parsing code in serviceability tools
Hi Egor,
Thank you for finding this problem. I created a new Jira issue [1] for that.
[1] https://bugs.openjdk.java.net/browse/JDK-8252933
Best regards,
Daniil
On 9/8/20, 8:31 AM, "Egor Ushakov" wrote:
Hi Daniil,
we have some tests checking the number of jdwp packets and they've
detected a regression here:
com.jetbrains.jdi.ObjectReferenceImpl#validateAssignment now always
requests referenceType (requiring one more jdwp packet if value is not
yet cached),
previously it happened only for arrays.
Simple fix like this solves the issue:
===
--- src/main/java/com/jetbrains/jdi/ObjectReferenceImpl.java (revision
d96cab0ec2eeb791cceb7884341c56496cc026b9)
+++ src/main/java/com/jetbrains/jdi/ObjectReferenceImpl.java (revision
10a78a742d17338778e0226990ad0029a71cbf50)
@@ -647,11 +647,10 @@
*/
JNITypeParser destSig = new
JNITypeParser(destination.signature());
-JNITypeParser sourceSig = new JNITypeParser(type().signature());
if (destSig.isPrimitive()) {
throw new InvalidTypeException("Can't assign object value
to primitive");
}
-if (destSig.isArray() && !sourceSig.isArray()) {
+if (destSig.isArray() && !new
JNITypeParser(type().signature()).isArray()) {
throw new InvalidTypeException("Can't assign non-array
value to an array");
}
if (destSig.isVoid()) {
If possible - please push the fix.
Thanks,
Egor
On 19-May-20 19:32, Daniil Titov wrote:
> Hi Chris and Serguei,
>
> Thank you for reviewing this change.
>
> Best regards,
> Daniil
>
> On 5/18/20, 12:41 PM, "Chris Plummer" wrote:
>
> Looks good.
>
> thanks,
>
> Chris
>
> On 5/14/20 1:33 PM, Daniil Titov wrote:
> > Hi Serguei and Chris,
> >
> > Please review a new version of the change [1] that addresses your
comments.
> >
> > Testing: Mach5 tier1-tier5 tests successfully passed.
> >
> > Regarding CR for the JDWP spec issues related to missing type
information in some of the protocol packet descriptions [3], as Chris has just
noticed
> > we really don't need it, so I withdrew this CR.
> >
> > [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02
> > [2] https://bugs.openjdk.java.net/browse/JDK-8241080
> > [3] https://bugs.openjdk.java.net/browse/JDK-8245057
> >
> > Thank you,
> > Daniil
> >
> >
> > From: "serguei.spit...@oracle.com"
> > Date: Monday, May 11, 2020 at 11:53 AM
> > To: Daniil Titov , serviceability-dev
> > Subject: Re: RFR: 8241080: Consolidate signature parsing code in
serviceability tools
> >
> > Hi Daniil,
> >
> > It looks pretty good in general.
> > A couple of nits below.
> >
> >
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html
> > +void *cursor;
> > +jbyte argumentTag;
> > +jint argIndex = 0;
> > +jvalue *argument = request->arguments;;
> > . . .
> > void *cursor;
> > jint argIndex = 0;
> > +jbyte argumentTag;
> > jvalue *argument = request->arguments;
> >
> > It is better if the local variables 'cursor' and 'argumentTag' get
initialized.
> > There is double semicolon: "arguments;;"
> >
> >
> >
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html
> >43 static inline jbyte basicType(const char *signature) {
> >
> > It'd be nice to rename it to basicTypeTag() to get it unified with
other functions below.
> >
> > It is more safe to run tier5 as well.
> >
> > Thanks,
> > Serguei
> >
> >
> > On 5/9/20 09:29, Daniil Titov wrote:
> > Please review a change[1] that centralizes the signature
processing in serviceability tools to make it capable of being easily
extensible in the future.
> >
> > Testin
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Hi Serguei and Alex,
Please review a new version of the webrev [1] that includes the changes
Serguei suggested. This version also has new test renamed from
DefaultMethods.java to OverpassMethods.java to avoid warnings during the build
about conflict with native library for runtime/jni/8033445/DefaultMethods.java
test.
[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.04/
Thank you,
Daniil
From: "serguei.spit...@oracle.com"
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov
, serviceability-dev
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of
default methods in super interfaces
Hi Daniil,
Thank you for the update!
It looks good to me.
Just two more minor comments and no need for another webrev you address them.
2519 int skipped = 0; // skip default methods
Saying "overpass methods" would be more precise.
47 printf("Enabled capability: maintain_original_method_order: true\n");
It is better to get rid of ": true" at the end (sorry, I missed this in my
previous suggestion).
Enabling capability means it is set to true.
Thanks,
Serguei
On 7/21/20 20:25, Daniil Titov wrote:
Hi Serguei and Alex,
Please, review new version of the change [1] that includes changes as Serguei
suggested.
114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
This method should not be included in the list of methods GetClassMethods
returns for the sub-interface or implementing class. I don't think we want to
comment each method in the test interfaces declared in this test when they
should be seen in this test and when they should not. This information already
exists in getclmthd007.cpp, thus I decided to omit this comment.
Please see below the output from the new test.
--messages:(4/215)--
command: main -agentlib:DefaultMethods DefaultMethods
reason: User specified action: run main/othervm/native -agentlib:DefaultMethods
DefaultMethods
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.241
--configuration:(0/0)--
--System.out:(3/265)--
Reflection getDeclaredMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.
--messages:(4/276)--
command: main -agentlib:DefaultMethods=maintain_original_method_order
DefaultMethods
reason: User specified action: run main/othervm/native
-agentlib:DefaultMethods=maintain_original_method_order DefaultMethods
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.25
--configuration:(0/0)--
--System.out:(4/322)--
Enabled capability: maintain_original_method_order: true
Reflection getDeclaredMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.
[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.02/
Thanks,
Daniil
From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov mailto:alexey.men...@oracle.com, Daniil Titov
mailto:daniil.x.ti...@oracle.com, serviceability-dev
mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of
default methods in super interfaces
Hi Daniil,
The fix looks pretty good to me.
Just minor comments.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
2519 int skipped = 0; // skip default methods from superinterface (see
JDK-8216324)
You can say just: // skip overpass methods
There is probably no need to list the bug number.
2523 // Depending on can_maintain_original_method_order capability
2524 // use the original method ordering indices stored in the class, so we
can emit
2525 // jmethodIDs in the order they appeared in the class file
2526 // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" => "or just copy in current order".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html
114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
117 interface OuterInterface1 extends DefaultInterface {
An extra space before "extends".
http://cr.openjdk.j
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Thank you, Serguei and Alex, for reviewing this change. I will apply suggested
corrections before pushing the fix.
Best regards,
Daniil
From: "serguei.spit...@oracle.com"
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov
, serviceability-dev
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of
default methods in super interfaces
Hi Daniil,
Thank you for the update!
It looks good to me.
Just two more minor comments and no need for another webrev you address them.
2519 int skipped = 0; // skip default methods
Saying "overpass methods" would be more precise.
47 printf("Enabled capability: maintain_original_method_order: true\n");
It is better to get rid of ": true" at the end (sorry, I missed this in my
previous suggestion).
Enabling capability means it is set to true.
Thanks,
Serguei
On 7/21/20 20:25, Daniil Titov wrote:
Hi Serguei and Alex,
Please, review new version of the change [1] that includes changes as Serguei
suggested.
114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
This method should not be included in the list of methods GetClassMethods
returns for the sub-interface or implementing class. I don't think we want to
comment each method in the test interfaces declared in this test when they
should be seen in this test and when they should not. This information already
exists in getclmthd007.cpp, thus I decided to omit this comment.
Please see below the output from the new test.
--messages:(4/215)--
command: main -agentlib:DefaultMethods DefaultMethods
reason: User specified action: run main/othervm/native -agentlib:DefaultMethods
DefaultMethods
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.241
--configuration:(0/0)--
--System.out:(3/265)--
Reflection getDeclaredMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.
--messages:(4/276)--
command: main -agentlib:DefaultMethods=maintain_original_method_order
DefaultMethods
reason: User specified action: run main/othervm/native
-agentlib:DefaultMethods=maintain_original_method_order DefaultMethods
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.25
--configuration:(0/0)--
--System.out:(4/322)--
Enabled capability: maintain_original_method_order: true
Reflection getDeclaredMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.
[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.02/
Thanks,
Daniil
From: "serguei.spit...@oracle.com"
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov , Daniil Titov
, serviceability-dev
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of
default methods in super interfaces
Hi Daniil,
The fix looks pretty good to me.
Just minor comments.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
2519 int skipped = 0; // skip default methods from superinterface (see
JDK-8216324)
You can say just: // skip overpass methods
There is probably no need to list the bug number.
2523 // Depending on can_maintain_original_method_order capability
2524 // use the original method ordering indices stored in the class, so we
can emit
2525 // jmethodIDs in the order they appeared in the class file
2526 // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" => "or just copy in current order".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html
114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
117 interface OuterInterface1 extends DefaultInterface {
An extra space before "extends".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/InterfaceDefMethods.java.html
I like the test simplicity.
Default methods are always in interfaces.
I'd suggest to rename the test to something like: DefaultMethods.java or
OverpassMethods.java.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serv
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Hi Serguei and Alex,
Please, review new version of the change [1] that includes changes as Serguei
suggested.
> 114 default void default_method() { } // should never be seen
> The comment above is not clear. Should never be seen in what context?
This method should not be included in the list of methods GetClassMethods
returns for the sub-interface or implementing class. I don't think we want to
comment each method in the test interfaces declared in this test when they
should be seen in this test and when they should not. This information already
exists in getclmthd007.cpp, thus I decided to omit this comment.
Please see below the output from the new test.
--messages:(4/215)--
command: main -agentlib:DefaultMethods DefaultMethods
reason: User specified action: run main/othervm/native -agentlib:DefaultMethods
DefaultMethods
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.241
--configuration:(0/0)--
--System.out:(3/265)--
Reflection getDeclaredMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.
--messages:(4/276)--
command: main -agentlib:DefaultMethods=maintain_original_method_order
DefaultMethods
reason: User specified action: run main/othervm/native
-agentlib:DefaultMethods=maintain_original_method_order DefaultMethods
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.25
--configuration:(0/0)--
--System.out:(4/322)--
Enabled capability: maintain_original_method_order: true
Reflection getDeclaredMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String
DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
--System.err:(1/15)--
STATUS:Passed.
[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.02/
Thanks,
Daniil
From: "serguei.spit...@oracle.com"
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov , Daniil Titov
, serviceability-dev
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of
default methods in super interfaces
Hi Daniil,
The fix looks pretty good to me.
Just minor comments.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
2519 int skipped = 0; // skip default methods from superinterface (see
JDK-8216324)
You can say just: // skip overpass methods
There is probably no need to list the bug number.
2523 // Depending on can_maintain_original_method_order capability
2524 // use the original method ordering indices stored in the class, so we
can emit
2525 // jmethodIDs in the order they appeared in the class file
2526 // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" => "or just copy in current order".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html
114 default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
117 interface OuterInterface1 extends DefaultInterface {
An extra space before "extends".
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/InterfaceDefMethods.java.html
I like the test simplicity.
Default methods are always in interfaces.
I'd suggest to rename the test to something like: DefaultMethods.java or
OverpassMethods.java.
http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/libInterfaceDefMethods.cpp.html
44 if (options != NULL && strcmp(options, "maintain_original_method_order")
== 0) {
45 printf("maintain_original_method_order: true\n");
...
54 } else {
55 printf("maintain_original_method_order: false\n");
I'd suggest to remove the lines 54 and 55 and adjust the line 45:
printf("Enabled capability: maintain_original_method_order: true\n");
88 jobject m = env->ToReflectedMethod(klass, methods[i], (modifiers & 8)
== 8);
It is better to replace 8 with a symbolic constant.
Thanks,
Serguei
On 7/20/20 16:57, Alex Menkov wrote:
Looks good to me :)
Thanks for handling this.
--alex
On 07/20/2020 12:00, Daniil Titov wrote:
Please review change [1] that fixes GetClassMethods behavior in cases if a
default method is present in a super interface. Currently for such cases the
information GetClass
RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Please review change [1] that fixes GetClassMethods behavior in cases if a default method is present in a super interface. Currently for such cases the information GetClassMethods returns for the sub-interface or implementing class incorrectly includes inherited not default methods. The fix ( thanks to Alex for providing this patch) ensures that overpass methods are filtered out in the returns. The fix also applies a change in the existing test as David suggested in the comments to the issue [2]. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8216324 Thank you, Daniil
Re: RFR: 8227337: javax/management/remote/mandatory/connection/ReconnectTest.java NoSuchObjectException no such object in table
Hi Paul, Thank you for reviewing this change. You are right, in most test configurations test.timeout.factor is greater than 1.0. Best regards, Daniil On 6/30/20, 8:50 AM, "Hohensee, Paul" wrote: The JBS issue is non-public, but this looks fine. I assume you set test.timeout.factor to something larger than 1.0 when you run MultiThreadDeadLockTest. Thanks, Paul On 6/29/20, 12:43 PM, "serviceability-dev on behalf of Daniil Titov" wrote: Please review the change that fixes an intermittent tests failure. The tests javax/management/remote/mandatory/connection/ReconnectTest.java and javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java use specific settings for server timeout that in some cases (e.g. when the test is run with -Xcomp) result in JMX server connection timeout thread unexports the remote object while the client connection is still in the progress. Below is an example of a such stacktrace: java.rmi.NoSuchObjectException: no such object in table at java.rmi/sun.rmi.transport.StreamRemoteCall.exceptionReceivedFromServer(StreamRemoteCall.java:303) at java.rmi/sun.rmi.transport.StreamRemoteCall.executeCall(StreamRemoteCall.java:279) at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:164) at jdk.remoteref/jdk.jmx.remote.internal.rmi.PRef.invoke(Unknown Source) at java.management.rmi/javax.management.remote.rmi.RMIConnectionImpl_Stub.getConnectionId(RMIConnectionImpl_Stub.java:318) at java.management.rmi/javax.management.remote.rmi.RMIConnector.getConnectionId(RMIConnector.java:385) at java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:347) at java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270) at MultiThreadDeadLockTest.main(MultiThreadDeadLockTest.java:87) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:564) at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) at java.base/java.lang.Thread.run(Thread.java:832) The fix adjusts the server timeout the tests use for "test.timeout.factor" system property. Testing: Mach5 tests are in the progress. [1] https://cr.openjdk.java.net/~dtitov/8227337/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8227337 Thanks, Daniil
Re: RFR(M): 8244383: jhsdb/HeapDumpTestWithActiveProcess.java fails with "AssertionFailure: illegal bci"
Hi Chris,
The fix, in general, looks good to me.
Some small comments for
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/amd64/AMD64CurrentFrameGuess.java:
1) Imports at lines 30 and 35 are not used and could be removed.
30 import sun.jvm.hotspot.interpreter.*;
35 import sun.jvm.hotspot.utilities.*;
2) Local variable "end" defined at line 189 is not used.
189 Address end = sp.addOffsetTo(regionInBytesToSearch);
No new webrev is required.
Thanks,
Daniil
On 6/25/20, 1:55 PM, "serviceability-dev on behalf of Chris Plummer"
wrote:
Ping #2. I still need one more reviewer (Thanks for the review, Dan). I
updated the webrev based on Dan's comments:
http://cr.openjdk.java.net/~cjplummer/8244383/webrev.01/
I can still make the simplification mentioned below if necessary.
thanks,
Chris
On 6/23/20 11:29 AM, Chris Plummer wrote:
> Ping!
>
> If this fix is too complicated, there is a simplification I can make,
> but at the cost of abandoning some attempts to determine the current
> frame when this error condition pops up. At the start of
> validateInterpreterFrame() it attempts to verify that the frame is
> valid by verifying that frame->method and frame->bcp are valid. This
> part is pretty simple. The complicated part is everything that follows
> if the verification fails. It attempts to error correct the situation
> by looking at various register contents and stack contents. I could
> just abandon this complicated code and return false if frame->method
> and frame->bcp don't check out. Upon return, the caller's code would
> be simplified to:
>
> if (validateInterpreterFrame(sp, fp, pc)) {
> return true; // We're done. setValues() has been called
> for valid interpreter frame.
> } else {
> return checkLastJavaSP();
> }
>
> So there's still a chance we can determine a valid current frame if
> "last java frame" has been setup. However, if not setup we would not
> be able to. This is where the complicated code in
> validateInterpreterFrame() is useful because it can usually determine
> the current frame, even if "last java frame" is not setup, but it's
> rare enough that we run into this situation that I think failing to
> get the current frame is ok.
>
> So if I can get a couple promises for reviews if I make this change,
> I'll go ahead and do it and send out a new RFR.
>
> thanks,
>
> Chris
>
> On 6/18/20 5:54 PM, Chris Plummer wrote:
>> [I've added runtime-dev to this SA review since understanding
>> interpreter invokes (code generated by
>> TemplateInterpreterGenerator::generate_normal_entry()) and stack
>> walking is probably more important than understanding SA.]
>>
>> Hello,
>>
>> Please help review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8244383
>> http://cr.openjdk.java.net/~cjplummer/8244383/webrev.00/index.html
>>
>> The crux of the bug is when doing stack walking the topmost frame is
>> in an inconsistent state because we are in the middle of pushing a
>> new interpreter frame. Basically we are executing code generated by
>> TemplateInterpreterGenerator::generate_normal_entry(). Since the PC
>> register is in this code, SA assumes the topmost frame is an
>> interpreter frame.
>>
>> The first issue with this interpreter frame assumption is if we
>> haven't actually pushed the frame yet, then the current frame is the
>> caller's frame, and could be compiled. But since SA thinks it's
>> interpreted, later on it tries to convert the frame->bcp to a BCI,
>> but frame->bcp is only valid for interpreter frames. Thus the
>> "illegal BCI" failures. If the previous frame happened to be
>> interpreted, then the existing SA code works fine.
>>
>> The other state of frame pushing that was problematic was when the
>> new frame had been pushed, but frame->method and frame->bcp were not
>> setup yet. This also would lead to "illegal BCI" later on because
>> garbage would be stored in these locations.
>>
>> Fixing the above problems requires trying to determine the state of
>> the frame push through a series of checks, and then adapting what is
>> considered to be the current frame based on the outcome of the
>> checks. The first things checked is that frame->method is valid (we
>> can successfully instantiate a wrapper for the Method* without
>> failure) and that frame->bcp is within the method. If both these pass
>> then we can use the frame as-is.
>>
>> If the above checks fail, then we try to determine whether the issue
>> is that the frame is not yet pushed and the current frame is
Re: RFR(S): 8247533: SA stack walking sometimes fails with sun.jvm.hotspot.debugger.DebuggerException: get_thread_regs failed for a lwp
Hi Chris, The fix looks good to me. Best regards, Daniil On 6/25/20 13:29, Chris Plummer wrote: > Ping. I still need one more review for this. There was one updated > webev. I list it below so you don't need to dig it up in the long > email thread: >> I've updated with webrev based on the new finding that a JavaThread >> cannot be on the ThreadList after its OS thread has been destroyed >> since the JavaThread removes itself from the ThreadList, and >> therefore must be running on its OS thread. The logic of the fix is >> unchanged from the first webrev, but I updated the comments to better >> reflect what is going on. I also updated the CR: >> >> https://bugs.openjdk.java.net/browse/JDK-8247533 >> http://cr.openjdk.java.net/~cjplummer/8247533/webrev.01/index.html > > thanks, > > Chris > > On 6/17/20 1:34 PM, Chris Plummer wrote: >> Hello, >> >> Please help review the following: >> >> https://bugs.openjdk.java.net/browse/JDK-8247533 >> http://cr.openjdk.java.net/~cjplummer/8247533/webrev.00/index.html >> >> The CR contains all the needed details. Here's a summary of changes >> in each file: >> >> src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp >> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m >> src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp >> -Instead of throwing an exception when the OS ThreadID is invalid, >> print a warning. >> >> src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c >> -Improve a print_debug message >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdThread.java >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxThread.java >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/windbg/amd64/WindbgAMD64Thread.java >> >> -Deal with the array of registers read in being null due to the OS >> ThreadID not being valid. >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java >> >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/linux/LinuxDebuggerLocal.java >> >> -Fix issue with "sun.jvm.hotspot.debugger.DebuggerException" >> appearing twice when printing the exception. >> >> thanks, >> >> Chris > >
RFR: 8205467: javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java possible deadlock
Please review a tiny change that adjusts the wait timeout the test uses for "test.timeout.factor" system property. Please note that a trivial merge with fix [4] that is currently on review [3] will be required. Since issues [2] and [4] describe different problems I decided to not combine these both changes in the single fix. Testing: Mach5 tests tier1-tier3 successfully passed. [1] Web rev: https://cr.openjdk.java.net/~dtitov/8205467/webrev.01/ [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8205467 [3] https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/032098.html [4] https://bugs.openjdk.java.net/browse/JDK-8227337 Thank you, Daniil
RFR: 8227337: javax/management/remote/mandatory/connection/ReconnectTest.java NoSuchObjectException no such object in table
Please review the change that fixes an intermittent tests failure. The tests javax/management/remote/mandatory/connection/ReconnectTest.java and javax/management/remote/mandatory/connection/MultiThreadDeadLockTest.java use specific settings for server timeout that in some cases (e.g. when the test is run with -Xcomp) result in JMX server connection timeout thread unexports the remote object while the client connection is still in the progress. Below is an example of a such stacktrace: java.rmi.NoSuchObjectException: no such object in table at java.rmi/sun.rmi.transport.StreamRemoteCall.exceptionReceivedFromServer(StreamRemoteCall.java:303) at java.rmi/sun.rmi.transport.StreamRemoteCall.executeCall(StreamRemoteCall.java:279) at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:164) at jdk.remoteref/jdk.jmx.remote.internal.rmi.PRef.invoke(Unknown Source) at java.management.rmi/javax.management.remote.rmi.RMIConnectionImpl_Stub.getConnectionId(RMIConnectionImpl_Stub.java:318) at java.management.rmi/javax.management.remote.rmi.RMIConnector.getConnectionId(RMIConnector.java:385) at java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:347) at java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270) at MultiThreadDeadLockTest.main(MultiThreadDeadLockTest.java:87) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:564) at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) at java.base/java.lang.Thread.run(Thread.java:832) The fix adjusts the server timeout the tests use for "test.timeout.factor" system property. Testing: Mach5 tests are in the progress. [1] https://cr.openjdk.java.net/~dtitov/8227337/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8227337 Thanks, Daniil
Re: RFR (S): 8245129: Enhance jstat gc option output and tests
Hi Paul, The change looks good to me. Thanks! --Daniil On 6/22/20, 8:48 AM, "serviceability-dev on behalf of Hohensee, Paul" wrote: Thanks very much for review, Volker. I'll file a follow-up issue. One more reviewer, please? :) Paul On 6/22/20, 8:10 AM, "serviceability-dev on behalf of Volker Simonis" wrote: Hi Paul, thanks for fixing jstat for larger heaps. I like that you've added explicit tests for ParallelGC which hasn't been tested since G1 was made the default collector. I also agree that sizes should all be right justified. I only wonder if the header of a right justified column shouldn't be right justified as well? However, taking into account that this already hasn't been handled consistently before your change, I'm fine to postpone that to a follow-up cleanup change. I think the change looks good so thumbs up from me. Thank you and best regards, Volker On Thu, Jun 18, 2020 at 11:53 PM Hohensee, Paul wrote: > > Ping. Any takers for this simple patch? > > > > Thanks, > > Paul > > > > From: serviceability-dev on behalf of "Hohensee, Paul" > Date: Monday, May 18, 2020 at 8:25 AM > To: serviceability-dev > Subject: RFR (S): 8245129: Enhance jstat gc option output and tests > > > > Please review an enhancement to the jstat gc option output to make the columns wider (for up to a 2TB heap) so one can read the output without going cross-eyed. > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8245129 > > Webrev: http://cr.openjdk.java.net/~phh/8245129/webrev.00/ > > > > I added tests using ParallelGC since the output can differ for non-G1 collectors. Successfully ran the test/hotspot/jtreg/serviceability/tmtools/jstat and test/jdk/sun/tools/jstat tests. A submit repo run had one failure > > > > runtime/MemberName/MemberNameLeak.java > > tier1 > > macosx-x64-debug > > > > but rerunning it on my laptop succeeded, and there’s no connection between this test and my patch. > > > > Thanks, > > Paul > > > >
Re: RFR: 8247469: getSystemCpuLoad() returns -1 on linux when some offline cpus are present and cpusets.effective_cpus is not available
Hi Matthias,
The change looks good to me. Probably it also makes sense to remove method
getHostConfiguredCpuCount0() since it is no longer used.
Thanks,
Daniil
On 6/12/20, 8:25 AM, "Baesken, Matthias" wrote:
Hello, please review the following change .
We have a Linux machine where
OperatingSystemMXBean mbean =
(com.sun.management.OperatingSystemMXBean)
ManagementFactory.getOperatingSystemMXBean();
double load = mbean.getSystemCpuLoad();
returns -1 ;
Reason is that there are offline CPUs (48 configured , 32 online ).
Additionally cpusets.effective_cpus is not available on that Linux system .
Bug/webrev :
https://bugs.openjdk.java.net/browse/JDK-8247466
http://cr.openjdk.java.net/~mbaesken/webrevs/8247469.0/
Thanks,
Matthias
-Original Message-
From: Bob Vandette
Sent: Freitag, 12. Juni 2020 15:02
To: Baesken, Matthias
Cc: daniil.x.ti...@oracle.com
Subject: Re: getCpuLoad() / getSystemCpuLoad() returns -1 on linux when
some offline cpus are present and cpusets.effective_cpus is not available
I looks like there are two problems here:
1. containerMetrics.getCpuSetCpus().length returns the online CPUs but
getHostConfiguredCpuCount0() returns the total number of CPUs including offline
ones.
One solution might be to add a getHostOnlineCpuCount0() function.
2. If getEffectiveCpuSetCpus is not available then we should use
getCpuSetCpus.
Bob.
> On Jun 12, 2020, at 6:43 AM, Baesken, Matthias
wrote:
>
> Hello, I noticed the following on one of our Linux machines :
>
> OperatingSystemMXBean mbean =
(com.sun.management.OperatingSystemMXBean)
ManagementFactory.getOperatingSystemMXBean();
> double load = mbean.getSystemCpuLoad();
>
> returns -1 ; this seems to be related to “8226575:
OperatingSystemMXBean should be made container aware” .
>
> This machine has the following “special features”
>
> - a few CPUs are offline (means the configured cpus are 48 but the online
cpus are 32) so
>
>
> private boolean isCpuSetSameAsHostCpuSet() {
> if (containerMetrics != null && containerMetrics.getCpuSetCpus()
!= null) {
> return containerMetrics.getCpuSetCpus().length ==
getHostConfiguredCpuCount0();
> }
> return false;
> }
>
> Returns false
>
> - the machine does not have cpusets.effective_cpus(not all Linux
machines have it )
>
> In this case getSystemCpuLoad() / getCpuLoad() returns -1 (because it
checks that 48 != 32, and next it checks for cpusets.effective_cpus which is
not present ).
>
> See the coding at :
>
https://hg.openjdk.java.net/jdk/jdk/file/bdc14b8d31ff/src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java#l136
>
> // If the cpuset is the same as the host's one there is
no need to iterate over each CPU
> if (isCpuSetSameAsHostCpuSet()) {
> return getCpuLoad0();
> } else {
> int[] cpuSet =
containerMetrics.getEffectiveCpuSetCpus();
> if (cpuSet != null && cpuSet.length > 0) {
> double systemLoad = 0.0;
> for (int cpu : cpuSet) {
> double cpuLoad = getSingleCpuLoad0(cpu);
> if (cpuLoad < 0) {
> return -1;
> }
> systemLoad += cpuLoad;
> }
> return systemLoad / cpuSet.length;
> }
> return -1;
> }
>
>
> Could we better a) return the native getCpuLoad0(); in this case or
b) use the available containerMetrics.getCpuSetCpus(); when
getEffectiveCpuSetCpus();
> Gives an empty array (btw. getCpuSetCpus() returns on this machine
the online cpus = 0,31 = 32 ) ?
>
> I opened
>
> https://bugs.openjdk.java.net/browse/JDK-8247469
>
> to track this (I see the issue in jdk/jdk but it seems it came
also to oracle jdk8u261, which the July update ).
>
> Best regards, Matthias
Re: RFR: 8246196: javax/management/MBeanServer/OldMBeanServerTest fails with AssertionError
Hi David and Alex, Thank for reviewing this change. I will push it to jdk15 repo as David suggested. Best regards, Daniil On 6/11/20, 5:28 PM, "Alex Menkov" wrote: +1 --alex On 06/11/2020 16:51, David Holmes wrote: > Hi Daniil, > > On 12/06/2020 5:56 am, Daniil Titov wrote: >> Please review change [1] that fixes an intermittent failure of the >> test when it is runs with -Xcomp. >> >> The problem here is that the timespan the test uses to count >> notifications is not adjusted for "test.timeout.factor" system property. > > The adjustment looks fine. > >> The original issue is reproducible in JDK 11 and on Solaris platform >> only. However, I think it makes sense to apply this change in JDK 15 >> to prevent this from possible happening in the future and then >> backport it to 11. > > Do you still intend this for 15 or just 16? If 15 then push to jdk15 > repo and it will get forward ported to jdk/jdk automatically. > > Thanks, > David > >> [1] http://cr.openjdk.java.net/~dtitov/8246196/webrev.01/ >> [2] https://bugs.openjdk.java.net/browse/JDK-8246196 >> >> Thank you, >> Daniil >> >> >>
RFR: 8246196: javax/management/MBeanServer/OldMBeanServerTest fails with AssertionError
Please review change [1] that fixes an intermittent failure of the test when it is runs with -Xcomp. The problem here is that the timespan the test uses to count notifications is not adjusted for "test.timeout.factor" system property. The original issue is reproducible in JDK 11 and on Solaris platform only. However, I think it makes sense to apply this change in JDK 15 to prevent this from possible happening in the future and then backport it to 11. [1] http://cr.openjdk.java.net/~dtitov/8246196/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8246196 Thank you, Daniil
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Thank you, Serguei!
I will add a comment before pushing the fix.
Best regards,
Daniil
On 6/4/20, 4:56 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
Got it, thanks.
I think, some short comment above this comparisons would be helpful.
LGTM.
Thanks,
Serguei
On 6/4/20 16:45, Daniil Titov wrote:
> Hi Serguei,
>
>> Note, the threads can be terminated even after the diff value is
>> calculated at line 203.
> Please note that the diff value calculated on line 203 shows how many
*test* threads were created or terminated,
> numNewThreads is number of new *test* threads and numTerminatedThreads is
number of terminated *test* threads.
>
> No *test* thread can terminate or start after the diff value is
calculated.
>
> Number of threads mbean.getThreadCount() could be seen as number of live
*test* threads plus number of live internal (non-test) threads,
> or A = B + C , where A - result of mbean.getThreadCount(), B - number of
live test threads, C - number of live non-test threads.
>
> Regardless what happens with internal "non-tested" threads the invariant
that this method tests is that number of threads
> mbean.getThreadCount() returns could not be less than number of live test
threads, or that A >= B.
>
>
> Best regards,
> Daniil
>
> On 6/4/20, 4:08 PM, "serguei.spit...@oracle.com"
wrote:
>
> Hi Daniil,
>
>
> On 6/4/20 16:01, Daniil Titov wrote:
> > Hi Serguei,
> >
> >> 201 private static void checkLiveThreads(int numNewThreads,
> >> 202 int
numTerminatedThreads) {
> >> 203 int diff = numNewThreads - numTerminatedThreads;
> >> 204 long threadCount = mbean.getThreadCount();
> >> 205 long expectedThreadCount = prevLiveTestThreadCount +
diff;
> >> 206 if (threadCount < expectedThreadCount) {
> >> 207 testFailed = true;
> >> When all threads are counted with mbean.getThreadCount() it is
not clear
> >> there is no race with new non-tested threads creation. Is it
possible?
> >> If so, then the check at line 206 is going to fail.
> > Even if some Internal (non-tested) threads are created the value
mbean.getThreadCount() returns should be no less than the expected number of
live test threads (please note that prevLiveTestThreadCount counts only *test*
threads) that means that condition on line 206 will be evaluated to *false* and
line 207 will not be executed and the test will pass.
>
> Okay, I see that it is failure condition.
> But then is there a race with (non-tested) threads termination?
> Note, the threads can be terminated even after the diff value is
> calculated at line 203.
> I'm sorry, if the same questions are repeated again.
>
> Thanks,
> Serguei
>
> > --Best regards,
> > Daniil
> >
> > From: "serguei.spit...@oracle.com"
> > Date: Thursday, June 4, 2020 at 3:03 PM
> > To: Daniil Titov , Alex Menkov
, serviceability-dev
> > Subject: Re: RFR: 8131745:
java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
> >
> > Hi Daniil,
> >
> > It is hard to be on top of all the details in these review rounds.
> > When all threads are counted with mbean.getThreadCount() it is not
clear
> > there is no race with new non-tested threads creation. Is it
possible?
> > If so, then the check at line 206 is going to fail.
> > 201 private static void checkLiveThreads(int numNewThreads,
> > 202 int
numTerminatedThreads) {
> > 203 int diff = numNewThreads - numTerminatedThreads;
> > 204 long threadCount = mbean.getThreadCount();
> > 205 long expectedThreadCount = prevLiveTestThreadCount +
diff;
> > 206 if (threadCount < expectedThreadCount) {
> > 207 testFailed = true;
> >
> > Thanks,
> > Serguei
> >
> > On 6/3/20 20:42, Daniil Titov wrote:
> > Hi Alex,
> >
> > Please review a new version of the webrev [1] that no longer uses
waitTillEquals() method.
>
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Serguei,
> Note, the threads can be terminated even after the diff value is
>calculated at line 203.
Please note that the diff value calculated on line 203 shows how many *test*
threads were created or terminated,
numNewThreads is number of new *test* threads and numTerminatedThreads is
number of terminated *test* threads.
No *test* thread can terminate or start after the diff value is calculated.
Number of threads mbean.getThreadCount() could be seen as number of live *test*
threads plus number of live internal (non-test) threads,
or A = B + C , where A - result of mbean.getThreadCount(), B - number of live
test threads, C - number of live non-test threads.
Regardless what happens with internal "non-tested" threads the invariant that
this method tests is that number of threads
mbean.getThreadCount() returns could not be less than number of live test
threads, or that A >= B.
Best regards,
Daniil
On 6/4/20, 4:08 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
On 6/4/20 16:01, Daniil Titov wrote:
> Hi Serguei,
>
>> 201 private static void checkLiveThreads(int numNewThreads,
>> 202 int numTerminatedThreads) {
>> 203 int diff = numNewThreads - numTerminatedThreads;
>> 204 long threadCount = mbean.getThreadCount();
>> 205 long expectedThreadCount = prevLiveTestThreadCount + diff;
>> 206 if (threadCount < expectedThreadCount) {
>> 207 testFailed = true;
>> When all threads are counted with mbean.getThreadCount() it is not clear
>> there is no race with new non-tested threads creation. Is it possible?
>> If so, then the check at line 206 is going to fail.
> Even if some Internal (non-tested) threads are created the value
mbean.getThreadCount() returns should be no less than the expected number of
live test threads (please note that prevLiveTestThreadCount counts only *test*
threads) that means that condition on line 206 will be evaluated to *false* and
line 207 will not be executed and the test will pass.
Okay, I see that it is failure condition.
But then is there a race with (non-tested) threads termination?
Note, the threads can be terminated even after the diff value is
calculated at line 203.
I'm sorry, if the same questions are repeated again.
Thanks,
Serguei
> --Best regards,
> Daniil
>
> From: "serguei.spit...@oracle.com"
> Date: Thursday, June 4, 2020 at 3:03 PM
> To: Daniil Titov , Alex Menkov
, serviceability-dev
> Subject: Re: RFR: 8131745:
java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
>
> Hi Daniil,
>
> It is hard to be on top of all the details in these review rounds.
> When all threads are counted with mbean.getThreadCount() it is not clear
> there is no race with new non-tested threads creation. Is it possible?
> If so, then the check at line 206 is going to fail.
> 201 private static void checkLiveThreads(int numNewThreads,
> 202 int numTerminatedThreads) {
> 203 int diff = numNewThreads - numTerminatedThreads;
> 204 long threadCount = mbean.getThreadCount();
> 205 long expectedThreadCount = prevLiveTestThreadCount + diff;
> 206 if (threadCount < expectedThreadCount) {
> 207 testFailed = true;
>
> Thanks,
> Serguei
>
> On 6/3/20 20:42, Daniil Titov wrote:
> Hi Alex,
>
> Please review a new version of the webrev [1] that no longer uses
waitTillEquals() method.
>
> [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.04/
> [2] https://bugs.openjdk.java.net/browse/JDK-8131745
>
> Thank you,
> Daniil
>
> On 6/3/20, 4:42 PM, "Alex Menkov" mailto:alexey.men...@oracle.com wrote:
>
> Hi again Daniil,
>
> On 06/03/2020 16:31, Daniil Titov wrote:
> > Hi Alex,
> >
> > Thanks for this suggestion. You are right, we actually don't need
this waitForAllThreads() method.
> >
> > I will include this change in the new version of the webrev.
> >
> >> 207 int diff = numNewThreads - numTerminatedThreads;
> >> 208 long threadCount = mbean.getThreadCount();
> >> 209 long expectedThreadCount =
prevLiveTestThreadCount + diff;
> >> 210 if (threadCount < expectedThreadCount) {
> >> if some interna
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Serguei,
> 201 private static void checkLiveThreads(int numNewThreads,
> 202 int numTerminatedThreads) {
> 203 int diff = numNewThreads - numTerminatedThreads;
> 204 long threadCount = mbean.getThreadCount();
> 205 long expectedThreadCount = prevLiveTestThreadCount + diff;
> 206 if (threadCount < expectedThreadCount) {
> 207 testFailed = true;
> When all threads are counted with mbean.getThreadCount() it is not clear
> there is no race with new non-tested threads creation. Is it possible?
> If so, then the check at line 206 is going to fail.
Even if some Internal (non-tested) threads are created the value
mbean.getThreadCount() returns should be no less than the expected number of
live test threads (please note that prevLiveTestThreadCount counts only *test*
threads) that means that condition on line 206 will be evaluated to *false* and
line 207 will not be executed and the test will pass.
--Best regards,
Daniil
From: "serguei.spit...@oracle.com"
Date: Thursday, June 4, 2020 at 3:03 PM
To: Daniil Titov , Alex Menkov
, serviceability-dev
Subject: Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java
still fails intermittently
Hi Daniil,
It is hard to be on top of all the details in these review rounds.
When all threads are counted with mbean.getThreadCount() it is not clear
there is no race with new non-tested threads creation. Is it possible?
If so, then the check at line 206 is going to fail.
201 private static void checkLiveThreads(int numNewThreads,
202 int numTerminatedThreads) {
203 int diff = numNewThreads - numTerminatedThreads;
204 long threadCount = mbean.getThreadCount();
205 long expectedThreadCount = prevLiveTestThreadCount + diff;
206 if (threadCount < expectedThreadCount) {
207 testFailed = true;
Thanks,
Serguei
On 6/3/20 20:42, Daniil Titov wrote:
Hi Alex,
Please review a new version of the webrev [1] that no longer uses
waitTillEquals() method.
[1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.04/
[2] https://bugs.openjdk.java.net/browse/JDK-8131745
Thank you,
Daniil
On 6/3/20, 4:42 PM, "Alex Menkov" mailto:alexey.men...@oracle.com wrote:
Hi again Daniil,
On 06/03/2020 16:31, Daniil Titov wrote:
> Hi Alex,
>
> Thanks for this suggestion. You are right, we actually don't need this
waitForAllThreads() method.
>
> I will include this change in the new version of the webrev.
>
>> 207 int diff = numNewThreads - numTerminatedThreads;
>> 208 long threadCount = mbean.getThreadCount();
>> 209 long expectedThreadCount = prevLiveTestThreadCount +
diff;
>> 210 if (threadCount < expectedThreadCount) {
>> if some internal thread terminates, we'll get failure here
>
> The failure will not happen. Please note that prevLiveTestThreadCount
counts only *test* threads. Thus even if some Internal threads terminated the
value mbean.getThreadCount() returns should still be no less than the expected
number of live test threads.
>
> 310 prevLiveTestThreadCount = getTestThreadCount();
Oh, yes, I missed it.
LGTM.
--alex
>
> Best regards,
> Daniil
>
>
> On 6/3/20, 3:08 PM, "Alex Menkov" mailto:alexey.men...@oracle.com wrote:
>
> Hi Daniil,
>
> couple notes:
>
> 198 waitForThreads(numNewThreads, numTerminatedThreads);
>
> You don't actually need any wait here.
> Test cases wait until all threads are in desired state
> (checkAllThreadsAlive uses startupCheck, checkDaemonThreadsDead and
> checkAllThreadsDead use join())
>
>
>205 private static void checkLiveThreads(int numNewThreads,
>206 int
numTerminatedThreads) {
>207 int diff = numNewThreads - numTerminatedThreads;
>208 long threadCount = mbean.getThreadCount();
>209 long expectedThreadCount = prevLiveTestThreadCount +
diff;
> 210 if (threadCount < expectedThreadCount) {
>
> if some internal thread terminates, we'll get failure here
>
>
> --alex
>
> On 06/02/2020 21:00, Daniil Titov wrote:
> > Hi Alex, Serguei, and Martin,
> >
> > Thank you for your comments. Please review a new version of the
fix that addresses them, specifically:
> > 1) Replaces a double loo
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Alex,
Please review a new version of the webrev [1] that no longer uses
waitTillEquals() method.
[1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.04/
[2] https://bugs.openjdk.java.net/browse/JDK-8131745
Thank you,
Daniil
On 6/3/20, 4:42 PM, "Alex Menkov" wrote:
Hi again Daniil,
On 06/03/2020 16:31, Daniil Titov wrote:
> Hi Alex,
>
> Thanks for this suggestion. You are right, we actually don't need this
waitForAllThreads() method.
>
> I will include this change in the new version of the webrev.
>
>> 207 int diff = numNewThreads - numTerminatedThreads;
>> 208 long threadCount = mbean.getThreadCount();
>> 209 long expectedThreadCount = prevLiveTestThreadCount +
diff;
>> 210 if (threadCount < expectedThreadCount) {
>> if some internal thread terminates, we'll get failure here
>
> The failure will not happen. Please note that prevLiveTestThreadCount
counts only *test* threads. Thus even if some Internal threads terminated the
value mbean.getThreadCount() returns should still be no less than the expected
number of live test threads.
>
> 310 prevLiveTestThreadCount = getTestThreadCount();
Oh, yes, I missed it.
LGTM.
--alex
>
> Best regards,
> Daniil
>
>
> On 6/3/20, 3:08 PM, "Alex Menkov" wrote:
>
> Hi Daniil,
>
> couple notes:
>
> 198 waitForThreads(numNewThreads, numTerminatedThreads);
>
> You don't actually need any wait here.
> Test cases wait until all threads are in desired state
> (checkAllThreadsAlive uses startupCheck, checkDaemonThreadsDead and
> checkAllThreadsDead use join())
>
>
>205 private static void checkLiveThreads(int numNewThreads,
>206 int
numTerminatedThreads) {
>207 int diff = numNewThreads - numTerminatedThreads;
>208 long threadCount = mbean.getThreadCount();
>209 long expectedThreadCount = prevLiveTestThreadCount +
diff;
>210 if (threadCount < expectedThreadCount) {
>
> if some internal thread terminates, we'll get failure here
>
>
> --alex
>
> On 06/02/2020 21:00, Daniil Titov wrote:
> > Hi Alex, Serguei, and Martin,
> >
> > Thank you for your comments. Please review a new version of the
fix that addresses them, specifically:
> > 1) Replaces a double loop in checkAllThreadsAlive() with a code
that uses collections and containsAll() method.
> > 2) Restores the checks for other ThreadMXBean methods
(getThreadCount(), getTotalStartedThreadCount(), getPeakThreadCount()) but with
more relaxed conditions.
> > 3) Relaxes the check inside checkThreadIds() method
> >
> >
> > [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.03/
> > [2] https://bugs.openjdk.java.net/browse/JDK-8131745
> >
> > Thank you,
> > Daniil
> >
> > On 6/1/20, 5:06 PM, "Alex Menkov"
wrote:
> >
> > Hi Daniil,
> >
> > 1. before the fix checkLiveThreads() tested
> > ThreadMXBean.getThreadCount(), but now as far as I see it
tests
> > Thread.getAllStackTraces();
> >
> > 2.
> >237 private static void checkThreadIds() throws
InterruptedException {
> >238 long[] list = mbean.getAllThreadIds();
> >239
> >240 waitTillEquals(
> >241 list.length,
> >242 ()->(long)mbean.getThreadCount(),
> >243 "Array length returned by " +
> >244 "getAllThreadIds() = %1$d not matched
count =
> > ${provided}",
> >245 ()->list.length
> >246 );
> >247 }
> >
> > I suppose purpose of waitTillEquals() is to handle
creation/termination
> > of VM internal threads.
> > But if some internal thread terminates after
mbean.getAllThreadIds() and
> > before 1st mbean.getThreadCount() call and then VM does n
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Alex,
Thanks for this suggestion. You are right, we actually don't need this
waitForAllThreads() method.
I will include this change in the new version of the webrev.
> 207 int diff = numNewThreads - numTerminatedThreads;
> 208 long threadCount = mbean.getThreadCount();
> 209 long expectedThreadCount = prevLiveTestThreadCount + diff;
> 210 if (threadCount < expectedThreadCount) {
>if some internal thread terminates, we'll get failure here
The failure will not happen. Please note that prevLiveTestThreadCount counts
only *test* threads. Thus even if some Internal threads terminated the value
mbean.getThreadCount() returns should still be no less than the expected
number of live test threads.
310 prevLiveTestThreadCount = getTestThreadCount();
Best regards,
Daniil
On 6/3/20, 3:08 PM, "Alex Menkov" wrote:
Hi Daniil,
couple notes:
198 waitForThreads(numNewThreads, numTerminatedThreads);
You don't actually need any wait here.
Test cases wait until all threads are in desired state
(checkAllThreadsAlive uses startupCheck, checkDaemonThreadsDead and
checkAllThreadsDead use join())
205 private static void checkLiveThreads(int numNewThreads,
206 int numTerminatedThreads) {
207 int diff = numNewThreads - numTerminatedThreads;
208 long threadCount = mbean.getThreadCount();
209 long expectedThreadCount = prevLiveTestThreadCount + diff;
210 if (threadCount < expectedThreadCount) {
if some internal thread terminates, we'll get failure here
--alex
On 06/02/2020 21:00, Daniil Titov wrote:
> Hi Alex, Serguei, and Martin,
>
> Thank you for your comments. Please review a new version of the fix that
addresses them, specifically:
> 1) Replaces a double loop in checkAllThreadsAlive() with a code that
uses collections and containsAll() method.
> 2) Restores the checks for other ThreadMXBean methods (getThreadCount(),
getTotalStartedThreadCount(), getPeakThreadCount()) but with more relaxed
conditions.
> 3) Relaxes the check inside checkThreadIds() method
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.03/
> [2] https://bugs.openjdk.java.net/browse/JDK-8131745
>
> Thank you,
> Daniil
>
> On 6/1/20, 5:06 PM, "Alex Menkov" wrote:
>
> Hi Daniil,
>
> 1. before the fix checkLiveThreads() tested
> ThreadMXBean.getThreadCount(), but now as far as I see it tests
> Thread.getAllStackTraces();
>
> 2.
>237 private static void checkThreadIds() throws
InterruptedException {
>238 long[] list = mbean.getAllThreadIds();
>239
>240 waitTillEquals(
>241 list.length,
>242 ()->(long)mbean.getThreadCount(),
>243 "Array length returned by " +
>244 "getAllThreadIds() = %1$d not matched count =
> ${provided}",
>245 ()->list.length
>246 );
>247 }
>
> I suppose purpose of waitTillEquals() is to handle
creation/termination
> of VM internal threads.
> But if some internal thread terminates after mbean.getAllThreadIds()
and
> before 1st mbean.getThreadCount() call and then VM does not need to
> restart it, waitTillEquals will wait forever.
>
> --alex
>
>
> On 05/29/2020 16:28, Daniil Titov wrote:
> > Hi Alex and Serguei,
> >
> > Please review a new version of the change [1] that makes sure that
the test counts
> > only the threads it creates and ignores Internal threads VM might
create or destroy.
> >
> > Testing: Running this test in Mach5 with Graal on several hundred
times ,
> > tier1-tier3 tests are in progress.
> >
> > [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.02/
> > [2] https://bugs.openjdk.java.net/browse/JDK-8131745
> >
> > Thank you,
> > Daniil
> >
> > On 5/22/20, 10:26 AM, "Alex Menkov"
wrote:
> >
> > Hi Daniil,
> >
> > I'm not sure all this retry logic is a good way.
> > As mentioned in jira the most important part of the testing
is ensuring
> > that you find all the crea
Re: RFR: 8081652: java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java timed out intermittently
Hi Chris,
> Do you think 60 seconds is a bit long? Isn't the expectation that the join
> should happen almost immediately or not at all?
In case if an exception is thrown in the try block after the thread is started
and before it is moved in the terminated state the join never happens at all.
And in other cases the join should happen immediately. I agree that 60 seconds
look as a bit long but I just wanted to minimize the odds that that we miss the
log and the root cause if the issue is reproduced on some slow machine with
such VM options as, say, -Xcomp.
>I don't think you need a separate bug, but you should document in the bug what
>currently can and can't be reproduce and what is being fixed.
I will update the bug with this information.
Best regards,
Daniil
From: Chris Plummer
Date: Wednesday, June 3, 2020 at 12:46 PM
To: Daniil Titov , David Holmes
, serviceability-dev
Subject: Re: RFR: 8081652:
java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java timed out
intermittently
Hi Daniil,
Ok, I misread getLog() and see that is now always returns the log. Do you think
60 seconds is a bit long? Isn't the expectation that the join should happen
almost immediately or not at all?
I don't think you need a separate bug, but you should document in the bug what
currently can and can't be reproduce and what is being fixed.
thanks,
Chris
On 6/3/20 12:08 PM, Daniil Titov wrote:
Hi Chris,
I was not able to reproduce the original issue anymore in Mach5. However, the
test itself has a potential for a deadlock (that was also reported) and in the
proposed change we fix it. The log still should be printed and the expectation
is that we will be able to see the underlying problem in it if it ever
reproduced.
I could create a separate bug ( not sure if the subtask is a good fit here
since the change fixes some problem in the test ) and close the current one as
not reproducible if you think it is a better approach.
Regarding Thread.suspend() and Thread.resume() methods the test also checks the
thread state after these methods are invoked and since these deprecated
methods are still in API I don’t think we should exclude them from being
tested.
Best regards,
Daniil
From: Chris Plummer
Date: Wednesday, June 3, 2020 at 11:40 AM
To: David Holmes , Daniil Titov
, serviceability-dev
Subject: Re: RFR: 8081652:
java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java timed out
intermittently
On 6/1/20 12:10 AM, David Holmes wrote:
Hi Daniil,
On 30/05/2020 10:07 am, Daniil Titov wrote:
Please review a change [1] that fixes an intermittent test timeout.
The main logic of the test has this basic structure:
try {
// lots of thread state manipulation of target
}
finally {
thread.getLog();
}
and as David noticed in his comment ( the last comment in [2] ) if an
exception occurs anywhere
in the try block we can hang waiting for the join() in getLog() because we
haven't executed the logic that
tells the thread to terminate.
So the fix puts a timeout on the join() which means the test will no longer
timeout but it will still fail when whatever was leading to the timeout now
happens. So as a diagnostic fix this seems fine. Hopefully the logger will show
what we need to see and determine the real underlying problem.
If this change is really just diagnostic in nature, then it should be a
subtask. However, it seems to me it will actually hide the failure. The test
won't get a timeout and won't print the log. Am I missing something?
Also, after reading through the bug comments it looks like the getLog()/join()
timeout issue is different from the main issue that caused the CR to be filed
in the first place. Comments regarding the initial problem are:
"According to the stack trace the test seems to hang on trying to load the
'java.lang.Math' class concurrently. "
"Need to see some native stacks to understand why the classloading thread is
not proceeding even though RUNNABLE."
"I should have looked at the test first - it uses Thread.suspend and
Thread.resume and so is inherently deadlock prone."
Does this issue no longer exist, or have we decided that since the test is
expected to be deadlock prone to just ignore it.
thanks,
Chris
Thanks,
David
-
Testing: Running a modified test that explicitly throws a runtime exception
inside the try block shows the fix solves the problem.
Mach5 tier1-tier3 tests passed. Mach5 tier4-tier5 tests are
in progress.
[1] http://cr.openjdk.java.net/~dtitov/8081652/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8081652
Thank you,
Daniil
Re: RFR: 8081652: java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java timed out intermittently
Hi Chris,
I was not able to reproduce the original issue anymore in Mach5. However, the
test itself has a potential for a deadlock (that was also reported) and in the
proposed change we fix it. The log still should be printed and the expectation
is that we will be able to see the underlying problem in it if it ever
reproduced.
I could create a separate bug ( not sure if the subtask is a good fit here
since the change fixes some problem in the test ) and close the current one as
not reproducible if you think it is a better approach.
Regarding Thread.suspend() and Thread.resume() methods the test also checks the
thread state after these methods are invoked and since these deprecated
methods are still in API I don’t think we should exclude them from being
tested.
Best regards,
Daniil
From: Chris Plummer
Date: Wednesday, June 3, 2020 at 11:40 AM
To: David Holmes , Daniil Titov
, serviceability-dev
Subject: Re: RFR: 8081652:
java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java timed out
intermittently
On 6/1/20 12:10 AM, David Holmes wrote:
Hi Daniil,
On 30/05/2020 10:07 am, Daniil Titov wrote:
Please review a change [1] that fixes an intermittent test timeout.
The main logic of the test has this basic structure:
try {
// lots of thread state manipulation of target
}
finally {
thread.getLog();
}
and as David noticed in his comment ( the last comment in [2] ) if an
exception occurs anywhere
in the try block we can hang waiting for the join() in getLog() because we
haven't executed the logic that
tells the thread to terminate.
So the fix puts a timeout on the join() which means the test will no longer
timeout but it will still fail when whatever was leading to the timeout now
happens. So as a diagnostic fix this seems fine. Hopefully the logger will show
what we need to see and determine the real underlying problem.
If this change is really just diagnostic in nature, then it should be a
subtask. However, it seems to me it will actually hide the failure. The test
won't get a timeout and won't print the log. Am I missing something?
Also, after reading through the bug comments it looks like the getLog()/join()
timeout issue is different from the main issue that caused the CR to be filed
in the first place. Comments regarding the initial problem are:
"According to the stack trace the test seems to hang on trying to load the
'java.lang.Math' class concurrently. "
"Need to see some native stacks to understand why the classloading thread is
not proceeding even though RUNNABLE."
"I should have looked at the test first - it uses Thread.suspend and
Thread.resume and so is inherently deadlock prone."
Does this issue no longer exist, or have we decided that since the test is
expected to be deadlock prone to just ignore it.
thanks,
Chris
Thanks,
David
-
Testing: Running a modified test that explicitly throws a runtime exception
inside the try block shows the fix solves the problem.
Mach5 tier1-tier3 tests passed. Mach5 tier4-tier5 tests are
in progress.
[1] http://cr.openjdk.java.net/~dtitov/8081652/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8081652
Thank you,
Daniil
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Alex, Serguei, and Martin,
Thank you for your comments. Please review a new version of the fix that
addresses them, specifically:
1) Replaces a double loop in checkAllThreadsAlive() with a code that uses
collections and containsAll() method.
2) Restores the checks for other ThreadMXBean methods (getThreadCount(),
getTotalStartedThreadCount(), getPeakThreadCount()) but with more relaxed
conditions.
3) Relaxes the check inside checkThreadIds() method
[1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.03/
[2] https://bugs.openjdk.java.net/browse/JDK-8131745
Thank you,
Daniil
On 6/1/20, 5:06 PM, "Alex Menkov" wrote:
Hi Daniil,
1. before the fix checkLiveThreads() tested
ThreadMXBean.getThreadCount(), but now as far as I see it tests
Thread.getAllStackTraces();
2.
237 private static void checkThreadIds() throws InterruptedException {
238 long[] list = mbean.getAllThreadIds();
239
240 waitTillEquals(
241 list.length,
242 ()->(long)mbean.getThreadCount(),
243 "Array length returned by " +
244 "getAllThreadIds() = %1$d not matched count =
${provided}",
245 ()->list.length
246 );
247 }
I suppose purpose of waitTillEquals() is to handle creation/termination
of VM internal threads.
But if some internal thread terminates after mbean.getAllThreadIds() and
before 1st mbean.getThreadCount() call and then VM does not need to
restart it, waitTillEquals will wait forever.
--alex
On 05/29/2020 16:28, Daniil Titov wrote:
> Hi Alex and Serguei,
>
> Please review a new version of the change [1] that makes sure that the
test counts
> only the threads it creates and ignores Internal threads VM might create
or destroy.
>
> Testing: Running this test in Mach5 with Graal on several hundred times ,
> tier1-tier3 tests are in progress.
>
> [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.02/
> [2] https://bugs.openjdk.java.net/browse/JDK-8131745
>
> Thank you,
> Daniil
>
> On 5/22/20, 10:26 AM, "Alex Menkov" wrote:
>
> Hi Daniil,
>
> I'm not sure all this retry logic is a good way.
> As mentioned in jira the most important part of the testing is
ensuring
> that you find all the created threads when they are alive, and you
don't
> find them when they are dead. The actual thread count checking is not
> that important.
> I agree with this and I'd just simplify the test by removing checks
for
> thread count. VM may create and destroy internal threads when it
needs it.
>
> --alex
>
> On 05/18/2020 10:31, Daniil Titov wrote:
> > Please review the change [1] that fixes an intermittent failure of
the test.
> >
> > This test creates and destroys a given number of daemon/user
threads and validates the count of those started/stopped threads against values
returned from ThreadMXBean thread counts. The problem here is that if some
internal threads is started ( e.g. " HotSpotGraalManagement Bean
Registration"), or destroyed (e.g. "JVMCI CompilerThread ") the test hangs
waiting for expected number of live threads.
> >
> > The fix limits the time the test is waiting for desired number of
live threads and in case if this limit is exceeded the test repeats itself.
> >
> > Testing. Test with Graal on and Mach5 tier1-tier7 test passed.
> >
> > [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.01
> > [2] https://bugs.openjdk.java.net/browse/JDK-8131745
> >
> > Thank you,
> > Daniil
> >
> >
> >
>
>
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Alex and Serguei, Please review a new version of the change [1] that makes sure that the test counts only the threads it creates and ignores Internal threads VM might create or destroy. Testing: Running this test in Mach5 with Graal on several hundred times , tier1-tier3 tests are in progress. [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.02/ [2] https://bugs.openjdk.java.net/browse/JDK-8131745 Thank you, Daniil On 5/22/20, 10:26 AM, "Alex Menkov" wrote: Hi Daniil, I'm not sure all this retry logic is a good way. As mentioned in jira the most important part of the testing is ensuring that you find all the created threads when they are alive, and you don't find them when they are dead. The actual thread count checking is not that important. I agree with this and I'd just simplify the test by removing checks for thread count. VM may create and destroy internal threads when it needs it. --alex On 05/18/2020 10:31, Daniil Titov wrote: > Please review the change [1] that fixes an intermittent failure of the test. > > This test creates and destroys a given number of daemon/user threads and validates the count of those started/stopped threads against values returned from ThreadMXBean thread counts. The problem here is that if some internal threads is started ( e.g. " HotSpotGraalManagement Bean Registration"), or destroyed (e.g. "JVMCI CompilerThread ") the test hangs waiting for expected number of live threads. > > The fix limits the time the test is waiting for desired number of live threads and in case if this limit is exceeded the test repeats itself. > > Testing. Test with Graal on and Mach5 tier1-tier7 test passed. > > [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.01 > [2] https://bugs.openjdk.java.net/browse/JDK-8131745 > > Thank you, > Daniil > > >
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Chris and David,
Thank you for reviewing this change.
--Best regards,
Daniil
On 5/26/20, 4:33 PM, "Chris Plummer" wrote:
Hi Daniil,
Looks good.
thanks,
Chris
On 5/26/20 10:46 AM, Daniil Titov wrote:
> Hi Chris and David,
>
> Please review a new version of the fix [1] with the changes Chris
suggested.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.02
> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
>
> Thank you,
> Daniil
>
> On 5/22/20, 11:50 AM, "Chris Plummer" wrote:
>
> Hi Daniil,
>
> There is one reference to "jvmwarningmsg" that occurs before it is
> declared while all the rest all come after. It probably would make
sense
> to move its declaration up near the top of the file.
>
> 92 private static void matchListedProcesses(OutputAnalyzer
output) {
> 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX)
> 94 .stderrShouldBeEmptyIgnoreVMWarnings();
> 95 }
>
> We probably use this coding pattern all over the place, but I think
it
> just leads to less readable code. Any reason not to use:
>
> 92 private static void matchListedProcesses(OutputAnalyzer
output) {
> 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX);
> 94 output.stderrShouldBeEmptyIgnoreVMWarnings();
> 95 }
>
> I just don't see the point of the chaining, and don't understand why
all
> these OutputAnalyzer methods return the "this" object in the first
> place. Maybe I'm missing something. I guess maybe there are cases
where
> the OutputAnalyzer object is not already in a local variable, adding
> some value to the chaining, but that's not the case here, and I
think if
> it were the case it would be more readable just to stick the
> OutputAnalyzer object in a local. There one other case of this:
>
>154 private static void matchPerfCounters(OutputAnalyzer
output) {
>155 output.stdoutShouldMatchByLine(PERF_COUNTER_REGEX,null,
>156 PERF_COUNTER_REGEX)
>157 .stderrShouldBeEmptyIgnoreVMWarnings();
>158 }
>
> I think you can also add spaces after the commas, and probably make
the
> first stdoutShouldMatchByLine() one line.
>
> thanks,
>
> Chris
>
> On 5/21/20 10:06 PM, Daniil Titov wrote:
> > Please review a webrev [1] that reverts the changes done in
jdk.test.lib.process.OutputAnalyzer in [3].
> >
> > Change [3] modified OutputAnalyzer
stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings
. The current webrev [1] reverts this change and instead makes the tests that
expects an empty stderr from launched j-* tools to filter out '-showversion'
from test options when forwarding test VM option to j*-tools.
> >
> > Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7
tests are in progress.
> >
> > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01
> > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
> > [3] https://bugs.openjdk.java.net/browse/JDK-8242009
> >
> > Thank you,
> > Daniil
> >
> >
>
>
>
>
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Chris and David,
Please review a new version of the fix [1] with the changes Chris suggested.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.02
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
Thank you,
Daniil
On 5/22/20, 11:50 AM, "Chris Plummer" wrote:
Hi Daniil,
There is one reference to "jvmwarningmsg" that occurs before it is
declared while all the rest all come after. It probably would make sense
to move its declaration up near the top of the file.
92 private static void matchListedProcesses(OutputAnalyzer output) {
93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX)
94 .stderrShouldBeEmptyIgnoreVMWarnings();
95 }
We probably use this coding pattern all over the place, but I think it
just leads to less readable code. Any reason not to use:
92 private static void matchListedProcesses(OutputAnalyzer output) {
93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX);
94 output.stderrShouldBeEmptyIgnoreVMWarnings();
95 }
I just don't see the point of the chaining, and don't understand why all
these OutputAnalyzer methods return the "this" object in the first
place. Maybe I'm missing something. I guess maybe there are cases where
the OutputAnalyzer object is not already in a local variable, adding
some value to the chaining, but that's not the case here, and I think if
it were the case it would be more readable just to stick the
OutputAnalyzer object in a local. There one other case of this:
154 private static void matchPerfCounters(OutputAnalyzer output) {
155 output.stdoutShouldMatchByLine(PERF_COUNTER_REGEX,null,
156 PERF_COUNTER_REGEX)
157 .stderrShouldBeEmptyIgnoreVMWarnings();
158 }
I think you can also add spaces after the commas, and probably make the
first stdoutShouldMatchByLine() one line.
thanks,
Chris
On 5/21/20 10:06 PM, Daniil Titov wrote:
> Please review a webrev [1] that reverts the changes done in
jdk.test.lib.process.OutputAnalyzer in [3].
>
> Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings()
methods to ignore also VM version strings . The current webrev [1] reverts this
change and instead makes the tests that expects an empty stderr from launched
j-* tools to filter out '-showversion' from test options when forwarding test
VM option to j*-tools.
>
> Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests
are in progress.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01
> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
> [3] https://bugs.openjdk.java.net/browse/JDK-8242009
>
> Thank you,
> Daniil
>
>
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Alex, I was thinking about it as well. But the test also indirectly tests getTotalStartedThreadCount(), getThreadCount(), and getPeakThreadCount() methods of ThreadMXBean. So I tried to not reduce the test coverage. Best regards, Daniil On 5/22/20, 10:26 AM, "Alex Menkov" wrote: Hi Daniil, I'm not sure all this retry logic is a good way. As mentioned in jira the most important part of the testing is ensuring that you find all the created threads when they are alive, and you don't find them when they are dead. The actual thread count checking is not that important. I agree with this and I'd just simplify the test by removing checks for thread count. VM may create and destroy internal threads when it needs it. --alex On 05/18/2020 10:31, Daniil Titov wrote: > Please review the change [1] that fixes an intermittent failure of the test. > > This test creates and destroys a given number of daemon/user threads and validates the count of those started/stopped threads against values returned from ThreadMXBean thread counts. The problem here is that if some internal threads is started ( e.g. " HotSpotGraalManagement Bean Registration"), or destroyed (e.g. "JVMCI CompilerThread ") the test hangs waiting for expected number of live threads. > > The fix limits the time the test is waiting for desired number of live threads and in case if this limit is exceeded the test repeats itself. > > Testing. Test with Graal on and Mach5 tier1-tier7 test passed. > > [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.01 > [2] https://bugs.openjdk.java.net/browse/JDK-8131745 > > Thank you, > Daniil > > >
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi David, Some tiers in Mach5 are configured to run tests with '-showversion' VM options. In JDK-8242009 [3] we started forwarding test VM options to j-*tools and some tests that launch them and expect an empty stderr (apart from VM warnings) need to be corrected to either ignore version messages in the output or to ensure that -showversion VM option is not passed to j*-tool even if it is passed to the test itself. Best regards, Daniil On 5/21/20, 10:41 PM, "David Holmes" wrote: Hi Dannil, On 22/05/2020 3:06 pm, Daniil Titov wrote: > Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. > > Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change Thank you for reverting the OutputAnalyzer change. > and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. That seems workable, though I'm unclear what is adding the "-showversion" in the first place? Thanks, David - > Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 > [3] https://bugs.openjdk.java.net/browse/JDK-8242009 > > Thank you, > Daniil > >
RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 [3] https://bugs.openjdk.java.net/browse/JDK-8242009 Thank you, Daniil
Re: RFR(S): 8244203: sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java fails with NullPointerException
Hi Chris,
I have a question about a change in
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/Dictionary.java.
61 /** All classes, and their initiating class loader, passed in. */
62 public void allEntriesDo(ClassLoaderDataGraph.ClassAndLoaderVisitor v,
Oop loader) {
63 int tblSize = tableSize();
64 for (int index = 0; index < tblSize; index++) {
65 for (DictionaryEntry probe = (DictionaryEntry) bucket(index); probe
!= null;
66 probe = (DictionaryEntry)
probe.next()) {
67 Klass k = probe.klass();
68 // Only visit InstanceKlasses that are at least in the "loaded"
init_state. Otherwise
69 // the InstanceKlass won't have some required fields initialized,
which can cause problems.
70 if (k instanceof InstanceKlass && ((InstanceKlass)k).isLoaded()) {
71 continue;
72 }
73 v.visit(k, loader);
74 }
75 }
76 }
Based on the comment at lines 68-69 should not condition on line 70 be
70 if (k instanceof InstanceKlass && !((InstanceKlass)k).isLoaded()) {
in order to we skip InstanceKlasses that are not in the "loaded" state?
Thank you,
Daniil
On 5/19/20, 12:47 PM, "serviceability-dev on behalf of Chris Plummer"
wrote:
Hello,
Please review the following:
http://cr.openjdk.java.net/~cjplummer/8244203/webrev.00/index.html
https://bugs.openjdk.java.net/browse/JDK-8244203
The root of the problem is that SA is trying iterate over all loaded
classes by using ClassLoaderDataGraph, but it possible that a class that
ClassLoaderDataGraph knows about can be in a state that makes it
findable by SA, but not yet initialized far enough to make is usable by SA.
The first issue I tracked down in this area was a case where an
InstanceKlass did not yet have its java_mirror. This resulted in the NPE
you see reported in the bug, because there is some SA code that assumes
all InstanceKlass's have a java_mirror. I fixed this by not having
ClassLoaderData.classesDo() return any InstanceKlass that was not yet
initialized to the point of being considered "loaded". That fixed this
particular problem, but there was another still lurking that was similar..
The second issue was that event attempting to iterate over the set of
loaded classes could cause an NPE, so you couldn't even get to the point
of being able to reject an InstanceKlass if it was not yet int he
"loaded" state. ClassLoaderData.classesDo() needs to get the first
Klass in its list:
public void classesDo(ClassLoaderDataGraph.ClassVisitor v) {
for (Klass l = getKlasses(); l != null; l = l.getNextLinkKlass()) {
v.visit(l);
}
}
Since the first Klass is the one most recently added, its also subject
to sometimes not yet being fully loaded. Calling getKlasses() will
instantiate (wrap) the first Klass in the list, which in this case is a
partially loaded InstanceKlass which was so early in its initialization
that InstanceKlass::_fields had not yet been setup. Creating an
InstanceKlass (on the SA side) requires _fields to be set, otherwise it
gets an NPE. I fixed this by allowing the InstanceKlass to still be
created if not yet "loaded", but skipped any further initialization of
the InstanceKlass that would rely on _fields. This allows the
InstanceKlass to still be used by ClassLoaderData.classesDo() to iterate
over the classes. However, I also wanted to make sure this uninitialized
InstanceKlass wasn't leaked out to SA beyond its use by classesDo(). It
looks like the only other way this is possible is with
ClassLoaderData.find() and Dictionary.allEntriesDo(), so I put an
InstanceKlass.isLoaded() checks there also.
One way I tested these fixes was to by introducing short sleep in
ClassFileParser::fill_instance_klass() right after the Klass gets added
to the ClassLoaderData:
_loader_data->add_class(ik, publicize);
+ os::naked_short_sleep(2);
By doing this the bug reproduced almost every time, and the fixes
resolved it.
You'll also notice a bug fix in ClassLoaderData.allEntriesDo(). The
outside loop is not only unnecessary, but results in the same Klass
being visited multiple times. It turns out no one uses this
functionality, but I fixed it anyway rather than rip it out because it
seemed it might be useful in the future.
The changes in ClhsdbClasses.java test are to better check for expected
classes when dumping the set of all classes. I added these after
realizing I had introduced a bug that caused only InstanceKlasses to be
dumped, not interfaces or arrays, so I added a few more types to the
list that we check.
thanks,
Chris
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Chris and Serguei,
Thank you for reviewing this change.
Best regards,
Daniil
On 5/18/20, 12:41 PM, "Chris Plummer" wrote:
Looks good.
thanks,
Chris
On 5/14/20 1:33 PM, Daniil Titov wrote:
> Hi Serguei and Chris,
>
> Please review a new version of the change [1] that addresses your
comments.
>
> Testing: Mach5 tier1-tier5 tests successfully passed.
>
> Regarding CR for the JDWP spec issues related to missing type information
in some of the protocol packet descriptions [3], as Chris has just noticed
> we really don't need it, so I withdrew this CR.
>
> [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02
> [2] https://bugs.openjdk.java.net/browse/JDK-8241080
> [3] https://bugs.openjdk.java.net/browse/JDK-8245057
>
> Thank you,
> Daniil
>
>
> From: "serguei.spit...@oracle.com"
> Date: Monday, May 11, 2020 at 11:53 AM
> To: Daniil Titov , serviceability-dev
> Subject: Re: RFR: 8241080: Consolidate signature parsing code in
serviceability tools
>
> Hi Daniil,
>
> It looks pretty good in general.
> A couple of nits below.
>
>
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html
> +void *cursor;
> +jbyte argumentTag;
> +jint argIndex = 0;
> +jvalue *argument = request->arguments;;
> . . .
> void *cursor;
> jint argIndex = 0;
> +jbyte argumentTag;
> jvalue *argument = request->arguments;
>
> It is better if the local variables 'cursor' and 'argumentTag' get
initialized.
> There is double semicolon: "arguments;;"
>
>
>
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html
>43 static inline jbyte basicType(const char *signature) {
>
> It'd be nice to rename it to basicTypeTag() to get it unified with other
functions below.
>
> It is more safe to run tier5 as well.
>
> Thanks,
> Serguei
>
>
> On 5/9/20 09:29, Daniil Titov wrote:
> Please review a change[1] that centralizes the signature processing in
serviceability tools to make it capable of being easily extensible in the
future.
>
> Testing: Mach5 tier1-tier3 tests successfully passed.
>
> [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01
> [2] https://bugs.openjdk.java.net/browse/JDK-8241080
>
> Thank you,
> Daniil
>
>
>
>
>
>
RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Please review the change [1] that fixes an intermittent failure of the test. This test creates and destroys a given number of daemon/user threads and validates the count of those started/stopped threads against values returned from ThreadMXBean thread counts. The problem here is that if some internal threads is started ( e.g. " HotSpotGraalManagement Bean Registration"), or destroyed (e.g. "JVMCI CompilerThread ") the test hangs waiting for expected number of live threads. The fix limits the time the test is waiting for desired number of live threads and in case if this limit is exceeded the test repeats itself. Testing. Test with Graal on and Mach5 tier1-tier7 test passed. [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8131745 Thank you, Daniil
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Serguei and Chris,
Please review a new version of the change [1] that addresses your comments.
Testing: Mach5 tier1-tier5 tests successfully passed.
Regarding CR for the JDWP spec issues related to missing type information in
some of the protocol packet descriptions [3], as Chris has just noticed
we really don't need it, so I withdrew this CR.
[1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.02
[2] https://bugs.openjdk.java.net/browse/JDK-8241080
[3] https://bugs.openjdk.java.net/browse/JDK-8245057
Thank you,
Daniil
From: "serguei.spit...@oracle.com"
Date: Monday, May 11, 2020 at 11:53 AM
To: Daniil Titov , serviceability-dev
Subject: Re: RFR: 8241080: Consolidate signature parsing code in serviceability
tools
Hi Daniil,
It looks pretty good in general.
A couple of nits below.
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c.udiff.html
+void *cursor;
+jbyte argumentTag;
+jint argIndex = 0;
+jvalue *argument = request->arguments;;
. . .
void *cursor;
jint argIndex = 0;
+jbyte argumentTag;
jvalue *argument = request->arguments;
It is better if the local variables 'cursor' and 'argumentTag' get initialized.
There is double semicolon: "arguments;;"
http://cr.openjdk.java.net/~dtitov/8241080/webrev.01/src/jdk.jdwp.agent/share/native/libjdwp/signature.h.html
43 static inline jbyte basicType(const char *signature) {
It'd be nice to rename it to basicTypeTag() to get it unified with other
functions below.
It is more safe to run tier5 as well.
Thanks,
Serguei
On 5/9/20 09:29, Daniil Titov wrote:
Please review a change[1] that centralizes the signature processing in
serviceability tools to make it capable of being easily extensible in the
future.
Testing: Mach5 tier1-tier3 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01
[2] https://bugs.openjdk.java.net/browse/JDK-8241080
Thank you,
Daniil
Re: RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests
Hi Chris, Thank you for reviewing this change. Best regards, Daniil On 5/11/20, 10:55 AM, "Chris Plummer" wrote: Hi Daniil, Looks good. thanks, Chris On 5/11/20 9:43 AM, Daniil Titov wrote: > Hi Chris, > > Please review a new version of the fix[1] that also updates jdk/sun/tools tests. > > Testing: Mach5 tier1-tier7 tests successfully passed. > > > [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.03/ > [2] ] https://bugs.openjdk.java.net/browse/JDK-8242009 > > Thank you, > Daniil > > On 5/8/20, 12:21 AM, "Chris Plummer" wrote: > > Hi Daniil, > > I just noticed you didn't update the tests in jdk/sun/tools/jhsdb. Do > you think these should be done also? > > Chris > > On 5/7/20 11:44 PM, Chris Plummer wrote: > > Hi Daniil, > > > > The changes look good. > > > > thanks, > > > > Chris > > > > On 5/4/20 1:05 PM, Daniil Titov wrote: > >> Hi Chris, > >> > >> Please review a new version of webrev [1] that addresses your comments. > >> > >> For the following 3 tests that showed the increase of the execution > >> time with -Xcomp > >> more than 5 minutes this version of the change strips -Xcomp option > >> when > >> forwarding VM arguments to j*-tools : > >> > >> -- serviceability/sa/sadebugd/SADebugDTest.java, > >> -- serviceability/sa/sadebugd/DebugdConnectTest.java, > >> -- serviceability/sa/ClhsdbJstackXcompStress.java > >> > >> The execution time for the rest of the tests when running with -Xcomp > >> was increased > >> within 1 and half minute. > >> > >> > >> [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.02/ > >> [2] https://bugs.openjdk.java.net/browse/JDK-8242009 > >> > >> Thank you, > >> Daniil > >> > >> > >> On 4/27/20, 12:55 PM, "Chris Plummer" wrote: > >> > >> Hi Daniil, > >> > >> Overall it looks good. A few comments below. > >> > >> Can you add a comment to TestSysProps.java indicating the reason > >> for > >> checking if the line starts with "[". > >> > >> In JDKToolLauncher you have an extra empty line: > >> > >>112 * Any platform specific arguments required for > >> running the > >> tool are > >>113 * automatically added. > >>114 * > >>115 * > >>116 * @param args > >> > >> In OutputAnalyzer.java, I wonder if all these matching APIs you > >> updated > >> should by default just include the version output in their > >> filtering. > >> > >> As for the added time to execute, I would suggest possibly > >> stripping > >> -Xcomp from the few outliers, and I would mostly focus on how much > >> longer it takes, not how many times longer. For example, > >> increasing from > >> 10 seconds to 40 seconds is not a big deal. Increasing from 10 > >> minutes > >> to 20 minutes is. > >> > >> SADebugDTest creates 8 tool processes so, that's probably the > >> reason for > >> the big increase, although I'm not sure why it is kind of slow > >> in the > >> first place. It does nothing more on each iteration than launch > >> "jhsdb > >> debugd", which will connect to the debuggee, and then is killed > >> off. > >> Maybe there is something slow with connecting, especial with RMI. > >> > >> thanks, > >> > >> Chris >
Re: RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests
Hi Chris,
Please review a new version of the fix[1] that also updates jdk/sun/tools
tests.
Testing: Mach5 tier1-tier7 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.03/
[2] ] https://bugs.openjdk.java.net/browse/JDK-8242009
Thank you,
Daniil
On 5/8/20, 12:21 AM, "Chris Plummer" wrote:
Hi Daniil,
I just noticed you didn't update the tests in jdk/sun/tools/jhsdb. Do
you think these should be done also?
Chris
On 5/7/20 11:44 PM, Chris Plummer wrote:
> Hi Daniil,
>
> The changes look good.
>
> thanks,
>
> Chris
>
> On 5/4/20 1:05 PM, Daniil Titov wrote:
>> Hi Chris,
>>
>> Please review a new version of webrev [1] that addresses your comments.
>>
>> For the following 3 tests that showed the increase of the execution
>> time with -Xcomp
>> more than 5 minutes this version of the change strips -Xcomp option
>> when
>> forwarding VM arguments to j*-tools :
>>
>> -- serviceability/sa/sadebugd/SADebugDTest.java,
>> -- serviceability/sa/sadebugd/DebugdConnectTest.java,
>> -- serviceability/sa/ClhsdbJstackXcompStress.java
>>
>> The execution time for the rest of the tests when running with -Xcomp
>> was increased
>> within 1 and half minute.
>>
>>
>> [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.02/
>> [2] https://bugs.openjdk.java.net/browse/JDK-8242009
>>
>> Thank you,
>> Daniil
>>
>>
>> On 4/27/20, 12:55 PM, "Chris Plummer" wrote:
>>
>> Hi Daniil,
>>
>> Overall it looks good. A few comments below.
>>
>> Can you add a comment to TestSysProps.java indicating the reason
>> for
>> checking if the line starts with "[".
>>
>> In JDKToolLauncher you have an extra empty line:
>>
>>112 * Any platform specific arguments required for
>> running the
>> tool are
>>113 * automatically added.
>>114 *
>>115 *
>>116 * @param args
>>
>> In OutputAnalyzer.java, I wonder if all these matching APIs you
>> updated
>> should by default just include the version output in their
>> filtering.
>>
>> As for the added time to execute, I would suggest possibly
>> stripping
>> -Xcomp from the few outliers, and I would mostly focus on how much
>> longer it takes, not how many times longer. For example,
>> increasing from
>> 10 seconds to 40 seconds is not a big deal. Increasing from 10
>> minutes
>> to 20 minutes is.
>>
>> SADebugDTest creates 8 tool processes so, that's probably the
>> reason for
>> the big increase, although I'm not sure why it is kind of slow
>> in the
>> first place. It does nothing more on each iteration than launch
>> "jhsdb
>> debugd", which will connect to the debuggee, and then is killed
>> off.
>> Maybe there is something slow with connecting, especial with RMI.
>>
>> thanks,
>>
>> Chris
>>
>> On 4/27/20 12:07 PM, Daniil Titov wrote:
>> > Please review the change [1] that ensures that VM and test
>> options are forwarded to
>> > j*-tools when they are launched from serviceability/sa tests.
>> >
>> > The tests that expect an empty output were corrected to
>> ignore the product version printed
>> > in the error stream since in some tiers the tests are run
>> with ' -showversion' VM option (tier3).
>> >
>> > In test serviceability/sa/TestSysProps.java the code that
>> counts the system properties was corrected
>> > to ignore the debug output when the test is run with "
>> -Xlog:cds=debug" option (tier4).
>> >
>> > Testing: Mach5 tests for tier1 - tier7 passed.
>> >
>> > I also run the test with -XComp at Mach5 linux-x64-debug
>> builds before and after the changes
>> > and for the most of the tests the overhead is about 2 times
>> although for
>> > serviceability/sa/sa
RFR: 8241080: Consolidate signature parsing code in serviceability tools
Please review a change[1] that centralizes the signature processing in serviceability tools to make it capable of being easily extensible in the future. Testing: Mach5 tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8241080/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8241080 Thank you, Daniil
RFR: 8194874 SA: Remove scripts with sa-jdi.jar dependencies.
Please review the change [1] that removes the scripts with dependencies to sa-jdi.jar that left after sa-jdi.jar was removed in JDK 9 [3]. Mach5 tier1-tier3 tests and doc build successfully passed. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8194874/webrev.01/ [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8194874 [3] https://bugs.openjdk.java.net/browse/JDK-8158050 Thank you, Daniil
Re: RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests
Hi Chris,
Yes. I think it makes sense to update these tests as well.
I will include them in the new version of the webrev.
Thank you,
Daniil
On 5/8/20, 12:21 AM, "Chris Plummer" wrote:
Hi Daniil,
I just noticed you didn't update the tests in jdk/sun/tools/jhsdb. Do
you think these should be done also?
Chris
On 5/7/20 11:44 PM, Chris Plummer wrote:
> Hi Daniil,
>
> The changes look good.
>
> thanks,
>
> Chris
>
> On 5/4/20 1:05 PM, Daniil Titov wrote:
>> Hi Chris,
>>
>> Please review a new version of webrev [1] that addresses your comments.
>>
>> For the following 3 tests that showed the increase of the execution
>> time with -Xcomp
>> more than 5 minutes this version of the change strips -Xcomp option
>> when
>> forwarding VM arguments to j*-tools :
>>
>> -- serviceability/sa/sadebugd/SADebugDTest.java,
>> -- serviceability/sa/sadebugd/DebugdConnectTest.java,
>> -- serviceability/sa/ClhsdbJstackXcompStress.java
>>
>> The execution time for the rest of the tests when running with -Xcomp
>> was increased
>> within 1 and half minute.
>>
>>
>> [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.02/
>> [2] https://bugs.openjdk.java.net/browse/JDK-8242009
>>
>> Thank you,
>> Daniil
>>
>>
>> On 4/27/20, 12:55 PM, "Chris Plummer" wrote:
>>
>> Hi Daniil,
>>
>> Overall it looks good. A few comments below.
>>
>> Can you add a comment to TestSysProps.java indicating the reason
>> for
>> checking if the line starts with "[".
>>
>> In JDKToolLauncher you have an extra empty line:
>>
>>112 * Any platform specific arguments required for
>> running the
>> tool are
>>113 * automatically added.
>>114 *
>>115 *
>>116 * @param args
>>
>> In OutputAnalyzer.java, I wonder if all these matching APIs you
>> updated
>> should by default just include the version output in their
>> filtering.
>>
>> As for the added time to execute, I would suggest possibly
>> stripping
>> -Xcomp from the few outliers, and I would mostly focus on how much
>> longer it takes, not how many times longer. For example,
>> increasing from
>> 10 seconds to 40 seconds is not a big deal. Increasing from 10
>> minutes
>> to 20 minutes is.
>>
>> SADebugDTest creates 8 tool processes so, that's probably the
>> reason for
>> the big increase, although I'm not sure why it is kind of slow
>> in the
>> first place. It does nothing more on each iteration than launch
>> "jhsdb
>> debugd", which will connect to the debuggee, and then is killed
>> off.
>> Maybe there is something slow with connecting, especial with RMI.
>>
>> thanks,
>>
>> Chris
>>
>> On 4/27/20 12:07 PM, Daniil Titov wrote:
>> > Please review the change [1] that ensures that VM and test
>> options are forwarded to
>> > j*-tools when they are launched from serviceability/sa tests.
>> >
>> > The tests that expect an empty output were corrected to
>> ignore the product version printed
>> > in the error stream since in some tiers the tests are run
>> with ' -showversion' VM option (tier3).
>> >
>> > In test serviceability/sa/TestSysProps.java the code that
>> counts the system properties was corrected
>> > to ignore the debug output when the test is run with "
>> -Xlog:cds=debug" option (tier4).
>> >
>> > Testing: Mach5 tests for tier1 - tier7 passed.
>> >
>> > I also run the test with -XComp at Mach5 linux-x64-debug
>> builds before and after the changes
>> > and for the most of the tests the overhead is about 2 times
>> although for
>> > serviceability/sa/sadebugd/SADebugDTest.java it spikes up to 5
>> times. Probably at least for some tests
>> > it makes to filter out some properties (e.g. -Xcomp) before
>> forwarding them to j*-tools.
>> >
>> > serviceability/sa/sadebugd/SADebugDTest.java, before : 2m 23s
>> , after:11m 07s
>> > serviceability/sa/sadebugd/TestJmapCore.java, before : 42s ,
>> after:1m 09s
>> > serviceability/sa/TestSysProps.java, before : 36s , after: 1m 27s
>> >
>> >
>> > [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.01
>> > [2] https://bugs.openjdk.java.net/browse/JDK-8242009
>> >
>> > Thank you,
>> > Daniil
>> >
>> >
>>
>>
>>
>>
>
>
Re: RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests
Hi Chris,
Please review a new version of webrev [1] that addresses your comments.
For the following 3 tests that showed the increase of the execution time with
-Xcomp
more than 5 minutes this version of the change strips -Xcomp option when
forwarding VM arguments to j*-tools :
-- serviceability/sa/sadebugd/SADebugDTest.java,
-- serviceability/sa/sadebugd/DebugdConnectTest.java,
-- serviceability/sa/ClhsdbJstackXcompStress.java
The execution time for the rest of the tests when running with -Xcomp was
increased
within 1 and half minute.
[1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.02/
[2] https://bugs.openjdk.java.net/browse/JDK-8242009
Thank you,
Daniil
On 4/27/20, 12:55 PM, "Chris Plummer" wrote:
Hi Daniil,
Overall it looks good. A few comments below.
Can you add a comment to TestSysProps.java indicating the reason for
checking if the line starts with "[".
In JDKToolLauncher you have an extra empty line:
112 * Any platform specific arguments required for running the
tool are
113 * automatically added.
114 *
115 *
116 * @param args
In OutputAnalyzer.java, I wonder if all these matching APIs you updated
should by default just include the version output in their filtering.
As for the added time to execute, I would suggest possibly stripping
-Xcomp from the few outliers, and I would mostly focus on how much
longer it takes, not how many times longer. For example, increasing from
10 seconds to 40 seconds is not a big deal. Increasing from 10 minutes
to 20 minutes is.
SADebugDTest creates 8 tool processes so, that's probably the reason for
the big increase, although I'm not sure why it is kind of slow in the
first place. It does nothing more on each iteration than launch "jhsdb
debugd", which will connect to the debuggee, and then is killed off.
Maybe there is something slow with connecting, especial with RMI.
thanks,
Chris
On 4/27/20 12:07 PM, Daniil Titov wrote:
> Please review the change [1] that ensures that VM and test options are
forwarded to
> j*-tools when they are launched from serviceability/sa tests.
>
> The tests that expect an empty output were corrected to ignore the
product version printed
> in the error stream since in some tiers the tests are run with '
-showversion' VM option (tier3).
>
> In test serviceability/sa/TestSysProps.java the code that counts the
system properties was corrected
> to ignore the debug output when the test is run with " -Xlog:cds=debug"
option (tier4).
>
> Testing: Mach5 tests for tier1 - tier7 passed.
>
> I also run the test with -XComp at Mach5 linux-x64-debug builds before
and after the changes
> and for the most of the tests the overhead is about 2 times although for
> serviceability/sa/sadebugd/SADebugDTest.java it spikes up to 5 times.
Probably at least for some tests
> it makes to filter out some properties (e.g. -Xcomp) before forwarding
them to j*-tools.
>
> serviceability/sa/sadebugd/SADebugDTest.java, before : 2m 23s ,
after:11m 07s
> serviceability/sa/sadebugd/TestJmapCore.java, before : 42s , after:1m
09s
> serviceability/sa/TestSysProps.java, before : 36s , after: 1m 27s
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.01
> [2] https://bugs.openjdk.java.net/browse/JDK-8242009
>
> Thank you,
> Daniil
>
>
Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same
Thank you, Chris and Serguei, for reviewing this change.
I will fix typos before pushing in the repository.
Best regards,
Daniil
On 4/27/20, 12:12 PM, "Chris Plummer" wrote:
Hi Daniil,
83 // names2, and names3 will have different size. Repeat
the test in this case.
Should be "sizes". There are a few instances of this in the comments
that need to be changed.
Looks good otherwise. I don't need to see another webrev.
thanks,
Chris
On 4/25/20 9:11 AM, Daniil Titov wrote:
> Hi Serguei,
>
> Please review a new version of the webrev [1] that changes the condition
> as you suggested. Please note then in both cases we need break from the
loop : the case when
> it is a first attempt and the conditions are met and the case when it is
a second attempt.
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.03
> [2] https://bugs.openjdk.java.net/browse/JDK-8242239
>
> Thank you,
> Daniil
>
>
> From: "serguei.spit...@oracle.com"
> Date: Saturday, April 25, 2020 at 12:06 AM
> To: Daniil Titov , Chris Plummer
, serviceability-dev
> Subject: Re: RFR: 8242239: [Graal]
javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets
same
>
> Hi Daniil,
>
> Thank you for the update.
>
> On 4/24/20 22:22, Daniil Titov wrote:
> Hi Chris and Serguei,
>
> Please review a new version of the fix [1] that makes the tests try to
repeat the check only once.
>
> Regarding the question Serguei asked:
>
> 97 while(true) {
> 113 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
> 114 assertEquals(mbeanCount * 2, counterCount);
> I doubt, the assert is really needed.
> we need the assert here to make the test fail in case if it is a second
attempt and the conditions are not met.
>
> A space is still needed in "while(true)."
> 111 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
> 112 assertEquals(mbeanCount * 2, counterCount);
> 113 break;
> 114 }
> The code above is a little confusing as we have to logically separate the
assert and break cases.
> Would something like below be more straightforward?:
> if (mbeanCount * 2 == counterCount) {
> break;
> }
> if (isSecondAttempt) {
> assertEquals(mbeanCount * 2 != counterCount);
> }
>
> Otherwise, the update looks good.
>
> Thanks,
> Serguei
>
>
> [1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.02/
> [2] https://bugs.openjdk.java.net/browse/JDK-8242239
>
> Thank you,
> Daniil
>
>
> From: serviceability-dev
mailto:serviceability-dev-boun...@openjdk.java.net on behalf of Daniil Titov
mailto:daniil.x.ti...@oracle.com
> Date: Friday, April 24, 2020 at 2:08 PM
> To: Chris Plummer mailto:chris.plum...@oracle.com,
mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com,
serviceability-dev mailto:serviceability-dev@openjdk.java.net
> Subject: Re: RFR: 8242239: [Graal]
javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets
same
>
> Hi Chris,
>
> I agree with you. I will change the test to retry just once.
>
> Thank you,
> Daniil
>
>
> From: Chris Plummer mailto:chris.plum...@oracle.com
> Date: Friday, April 24, 2020 at 1:27 PM
> To: Daniil Titov mailto:daniil.x.ti...@oracle.com,
mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com,
serviceability-dev mailto:serviceability-dev@openjdk.java.net
> Subject: Re: RFR: 8242239: [Graal]
javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets
same
>
> Hi Daniil,
>
> I think a retry count more in line with our current expectations would
make more sense since it is deterministic how many retries are might actually
be needed. You just used a large number in case more “lazy-registered” mbeans
are added in the future, but if this is the case then maybe even 10 is not
enough.
>
> thanks,
>
> Chris
>
> On 4/24/20 12:36 PM, Daniil Titov wrote:
> Hi Serguei and Chris,
>
> Thank you for your comments.
>
> I wanted to make the fix more generic and not limit it just to Graal
management bean. In this case we could expect that after the first retry is
triggered by Graal MBean registration some other “lazy-registered”
RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests
Please review the change [1] that ensures that VM and test options are forwarded to j*-tools when they are launched from serviceability/sa tests. The tests that expect an empty output were corrected to ignore the product version printed in the error stream since in some tiers the tests are run with ' -showversion' VM option (tier3). In test serviceability/sa/TestSysProps.java the code that counts the system properties was corrected to ignore the debug output when the test is run with " -Xlog:cds=debug" option (tier4). Testing: Mach5 tests for tier1 - tier7 passed. I also run the test with -XComp at Mach5 linux-x64-debug builds before and after the changes and for the most of the tests the overhead is about 2 times although for serviceability/sa/sadebugd/SADebugDTest.java it spikes up to 5 times. Probably at least for some tests it makes to filter out some properties (e.g. -Xcomp) before forwarding them to j*-tools. serviceability/sa/sadebugd/SADebugDTest.java, before : 2m 23s , after:11m 07s serviceability/sa/sadebugd/TestJmapCore.java, before : 42s , after:1m 09s serviceability/sa/TestSysProps.java, before : 36s , after: 1m 27s [1] http://cr.openjdk.java.net/~dtitov/8242009/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8242009 Thank you, Daniil
Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same
Hi Serguei,
Please review a new version of the webrev [1] that changes the condition
as you suggested. Please note then in both cases we need break from the loop :
the case when
it is a first attempt and the conditions are met and the case when it is a
second attempt.
[1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.03
[2] https://bugs.openjdk.java.net/browse/JDK-8242239
Thank you,
Daniil
From: "serguei.spit...@oracle.com"
Date: Saturday, April 25, 2020 at 12:06 AM
To: Daniil Titov , Chris Plummer
, serviceability-dev
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Daniil,
Thank you for the update.
On 4/24/20 22:22, Daniil Titov wrote:
Hi Chris and Serguei,
Please review a new version of the fix [1] that makes the tests try to repeat
the check only once.
Regarding the question Serguei asked:
97 while(true) {
113 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
114 assertEquals(mbeanCount * 2, counterCount);
I doubt, the assert is really needed.
we need the assert here to make the test fail in case if it is a second attempt
and the conditions are not met.
A space is still needed in "while(true)."
111 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
112 assertEquals(mbeanCount * 2, counterCount);
113 break;
114 }
The code above is a little confusing as we have to logically separate the
assert and break cases.
Would something like below be more straightforward?:
if (mbeanCount * 2 == counterCount) {
break;
}
if (isSecondAttempt) {
assertEquals(mbeanCount * 2 != counterCount);
}
Otherwise, the update looks good.
Thanks,
Serguei
[1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.02/
[2] https://bugs.openjdk.java.net/browse/JDK-8242239
Thank you,
Daniil
From: serviceability-dev mailto:serviceability-dev-boun...@openjdk.java.net on
behalf of Daniil Titov mailto:daniil.x.ti...@oracle.com
Date: Friday, April 24, 2020 at 2:08 PM
To: Chris Plummer mailto:chris.plum...@oracle.com,
mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com,
serviceability-dev mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Chris,
I agree with you. I will change the test to retry just once.
Thank you,
Daniil
From: Chris Plummer mailto:chris.plum...@oracle.com
Date: Friday, April 24, 2020 at 1:27 PM
To: Daniil Titov mailto:daniil.x.ti...@oracle.com,
mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com,
serviceability-dev mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Daniil,
I think a retry count more in line with our current expectations would make
more sense since it is deterministic how many retries are might actually be
needed. You just used a large number in case more “lazy-registered” mbeans are
added in the future, but if this is the case then maybe even 10 is not enough.
thanks,
Chris
On 4/24/20 12:36 PM, Daniil Titov wrote:
Hi Serguei and Chris,
Thank you for your comments.
I wanted to make the fix more generic and not limit it just to Graal
management bean. In this case we could expect that after the first retry is
triggered by Graal MBean registration some other “lazy-registered” MBean got
registered and the test might fail. To avoid this we need to allow test to make
at least several retry attempts before failing. If number 10 looks as too high
we could make it 5 or even 3. This should not affect the test run-time unless
the test starts failing for some other reason than we expect. In the new
webrev I will also correct the iteration counting (as Chris mentioned) and fix
typos.
Thanks again,
Daniil
From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Friday, April 24, 2020 at 11:48 AM
To: Chris Plummer mailto:chris.plum...@oracle.com, Daniil Titov
mailto:daniil.x.ti...@oracle.com, serviceability-dev
mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Daniil,
Besides what Chris is pointed out the fix looks good.
Minor:
97 while(true) {
113 if(mbeanCount * 2 == counterCount || retryCounter++ >
MAX_RETRY_ATTEMPT) {
114 assertEquals(mbeanCount * 2, counterCount);
Space is missed in while and if.
I doubt, the assert is really needed.
96 // is running ( e.g. Graal MBean). In this case just retry the
test.
Extra space before "e.g.".
Thanks,
Serguei
On 4/24/20 11:30, Chris Plummer wrote:
Hi Daniil,
84 // If new MBean (e.g
Re: RFR: 8238561 serviceability/sa tests continue to run out of memory on Win* machines
Hi Leonid, I run the test with -XComp at Mach5 linux-x64-debug builds before and after the changes the forwards VM and test options to j-* tools and here is the difference in the execution times for some serviceability/sa tests: serviceability/sa/sadebugd/SADebugDTest.java, before : 2m 23s , after:11m 07s serviceability/sa/sadebugd/TestJmapCore.java, before : 42s , after:1m 09s serviceability/sa/TestSysProps.java, before : 36s , after:1m 27s Thus for the most of the tests the overhead is about 2 times although in case of serviceability/sa/sadebugd/SADebugDTest.java it spikes up to 5 times. Best regards, Daniil On 4/23/20, 11:23 AM, "Leonid Mesnik" wrote: Hi Daniil Have you checked how longer tests are executed with adding java options to launchers (with Xcomp/Graal/ZGC)? If the overhead is not significant might be it make worth to add options by default? The idea is to add vm.opts to all processes and java.opts to tested only. But if you believe that adding java.opts to jdk tools helps to improve testing then it is better just to add it by default. It helps to avoid similar failures in the future or with highly concurrent execution (when tens of jaotc are started for example). Leonid On 4/23/20 11:10 AM, Chris Plummer wrote: > On 4/23/20 10:18 AM, Daniil Titov wrote: >> Hi Chris, >> >> I will revoke this RFR and resubmit it under JDK-8242009 >> with the changes you suggested to use Utils.getTestJavaOpts() >> and make JDKToolLauncher to have an option to forward VM options. >> > It could also be done with a new API such as > JDKToolLauncher.addVMArgs(). That might be better than a new "create" > API. > > thank,s > > Chris >>> Is your change causing -Xshowversion to be passed? >> Yes, the changes makes tests run with -Xshowversion to be passed. >> >>> Do you know where it is coming from? >> It is coming from task definitions for different tiers. >> >> Thank you, >> Daniil >> >> On 4/22/20, 12:54 PM, "Chris Plummer" wrote: >> >> Hi Daniil, >> >> Thanks for cleaning this up. I think this should be fixed under >> JDK-8242009. JDK-8238561 involves more than just this one issue. >> >> Is there a reason why you didn't just change JDKToolLauncher to >> have an >> option or API to add the args? >> >> Why are you calling Utils.addTestJavaOpts() instead of >> Utils.getTestJavaOpts()? >> >> Is your change causing -Xshowversion to be passed? Do you know >> where it >> is coming from? >> >> thanks, >> >> Chris >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8242009 >> >> On 4/22/20 10:48 AM, Daniil Titov wrote: >> > Please review the change [1] that ensures that VM and test >> options are forwarded to >> > j*-tools when they are launched from serviceability/sa tests. >> > >> > In particular, it will ensure that passed to the tests maximum >> heap size settings ( -XX:MaxRAMPercentage) >> > are also honored by j*-tools serviceability/sa tests launch. >> > >> > The tests that expect an empty output were corrected to >> ignore the product version printed >> > in the error stream since in some tiers the tests are run >> with ' -showversion' VM option. >> > >> > Testing: Mach5 tests for tier1 - tier7 passed. >> > >> > [1] http://cr.openjdk.java.net/~dtitov/8238561/webrev.01 >> > [2] https://bugs.openjdk.java.net/browse/JDK-8238561 >> > >> > Thank you, >> > Daniil >> > >> > >> >> >> > >
Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same
Hi Chris and Serguei,
Please review a new version of the fix [1] that makes the tests try to repeat
the check only once.
Regarding the question Serguei asked:
> 97 while(true) {
> 113 if (mbeanCount * 2 == counterCount || isSecondAttempt) {
> 114 assertEquals(mbeanCount * 2, counterCount);
> I doubt, the assert is really needed.
we need the assert here to make the test fail in case if it is a second attempt
and the conditions are not met.
[1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.02/
[2] https://bugs.openjdk.java.net/browse/JDK-8242239
Thank you,
Daniil
From: serviceability-dev on
behalf of Daniil Titov
Date: Friday, April 24, 2020 at 2:08 PM
To: Chris Plummer , "serguei.spit...@oracle.com"
, serviceability-dev
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Chris,
I agree with you. I will change the test to retry just once.
Thank you,
Daniil
From: Chris Plummer
Date: Friday, April 24, 2020 at 1:27 PM
To: Daniil Titov , "serguei.spit...@oracle.com"
, serviceability-dev
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Daniil,
I think a retry count more in line with our current expectations would make
more sense since it is deterministic how many retries are might actually be
needed. You just used a large number in case more “lazy-registered” mbeans are
added in the future, but if this is the case then maybe even 10 is not enough.
thanks,
Chris
On 4/24/20 12:36 PM, Daniil Titov wrote:
Hi Serguei and Chris,
Thank you for your comments.
I wanted to make the fix more generic and not limit it just to Graal
management bean. In this case we could expect that after the first retry is
triggered by Graal MBean registration some other “lazy-registered” MBean got
registered and the test might fail. To avoid this we need to allow test to make
at least several retry attempts before failing. If number 10 looks as too high
we could make it 5 or even 3. This should not affect the test run-time unless
the test starts failing for some other reason than we expect. In the new
webrev I will also correct the iteration counting (as Chris mentioned) and fix
typos.
Thanks again,
Daniil
From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Friday, April 24, 2020 at 11:48 AM
To: Chris Plummer mailto:chris.plum...@oracle.com, Daniil Titov
mailto:daniil.x.ti...@oracle.com, serviceability-dev
mailto:serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Daniil,
Besides what Chris is pointed out the fix looks good.
Minor:
97 while(true) {
113 if(mbeanCount * 2 == counterCount || retryCounter++ >
MAX_RETRY_ATTEMPT) {
114 assertEquals(mbeanCount * 2, counterCount);
Space is missed in while and if.
I doubt, the assert is really needed.
96 // is running ( e.g. Graal MBean). In this case just retry the
test.
Extra space before "e.g.".
Thanks,
Serguei
On 4/24/20 11:30, Chris Plummer wrote:
Hi Daniil,
84 // If new MBean (e.g. Graal MBean) is registred while the test
is running names1,
106 // If new MBean (e.g. Graal MBean) is registred while the test
is running mbeans1,
registred -> registered
',' after "running"
Just wondering how you chose 10 as the number of retries. Seems excessive.
Shouldn't the problem turn up at most 1 time and therefore only 1 retry is
needed.
76 int counter = 0;
86 if (sameSize(names1, names2, names3) || counter++ >
MAX_RETRY_ATTEMPTS) {
The way the checks are done you will actually end up retrying
MAX_RETRY_ATTEMPTS+1 times. For example, if MAX_RETRY_ATTEMPTS is 1, first time
through the loop counter is 0 so a retry is allowed. Second time through the
loop counter is 1, so a retry is allowed again.
thanks,
Chris
On 4/18/20 11:30 AM, Daniil Titov wrote:
Please review the change that fixes the failure of
javax/management/generified/GenericTest.java and
javax/management/query/CustomQueryTest.java tests when Graal is on.
The tests checks that calls of management API are consistent and return the
same set of MBeans. However,
when Graal is on the Graal MBean might be registered between the calls that in
turn makes the results
inconsistent and the tests fail.
The fix makes the test aware that some MBean might be registered while the
checks run and if it happens the tests repeat the check.
Testing : Mach5 tests with Graal on and tier1-tier3 tests passed.
[1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8242239
Thanks,
Daniil
Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same
Hi Chris,
I agree with you. I will change the test to retry just once.
Thank you,
Daniil
From: Chris Plummer
Date: Friday, April 24, 2020 at 1:27 PM
To: Daniil Titov , "serguei.spit...@oracle.com"
, serviceability-dev
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Daniil,
I think a retry count more in line with our current expectations would make
more sense since it is deterministic how many retries are might actually be
needed. You just used a large number in case more “lazy-registered” mbeans are
added in the future, but if this is the case then maybe even 10 is not enough.
thanks,
Chris
On 4/24/20 12:36 PM, Daniil Titov wrote:
Hi Serguei and Chris,
Thank you for your comments.
I wanted to make the fix more generic and not limit it just to Graal
management bean. In this case we could expect that after the first retry is
triggered by Graal MBean registration some other “lazy-registered” MBean got
registered and the test might fail. To avoid this we need to allow test to make
at least several retry attempts before failing. If number 10 looks as too high
we could make it 5 or even 3. This should not affect the test run-time unless
the test starts failing for some other reason than we expect. In the new
webrev I will also correct the iteration counting (as Chris mentioned) and fix
typos.
Thanks again,
Daniil
From: "serguei.spit...@oracle.com"
Date: Friday, April 24, 2020 at 11:48 AM
To: Chris Plummer , Daniil Titov
, serviceability-dev
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Daniil,
Besides what Chris is pointed out the fix looks good.
Minor:
97 while(true) {
113 if(mbeanCount * 2 == counterCount || retryCounter++ >
MAX_RETRY_ATTEMPT) {
114 assertEquals(mbeanCount * 2, counterCount);
Space is missed in while and if.
I doubt, the assert is really needed.
96 // is running ( e.g. Graal MBean). In this case just retry the
test.
Extra space before "e.g.".
Thanks,
Serguei
On 4/24/20 11:30, Chris Plummer wrote:
Hi Daniil,
84 // If new MBean (e.g. Graal MBean) is registred while the test
is running names1,
106 // If new MBean (e.g. Graal MBean) is registred while the test
is running mbeans1,
registred -> registered
',' after "running"
Just wondering how you chose 10 as the number of retries. Seems excessive.
Shouldn't the problem turn up at most 1 time and therefore only 1 retry is
needed.
76 int counter = 0;
86 if (sameSize(names1, names2, names3) || counter++ >
MAX_RETRY_ATTEMPTS) {
The way the checks are done you will actually end up retrying
MAX_RETRY_ATTEMPTS+1 times. For example, if MAX_RETRY_ATTEMPTS is 1, first time
through the loop counter is 0 so a retry is allowed. Second time through the
loop counter is 1, so a retry is allowed again.
thanks,
Chris
On 4/18/20 11:30 AM, Daniil Titov wrote:
Please review the change that fixes the failure of
javax/management/generified/GenericTest.java and
javax/management/query/CustomQueryTest.java tests when Graal is on.
The tests checks that calls of management API are consistent and return the
same set of MBeans. However,
when Graal is on the Graal MBean might be registered between the calls that in
turn makes the results
inconsistent and the tests fail.
The fix makes the test aware that some MBean might be registered while the
checks run and if it happens the tests repeat the check.
Testing : Mach5 tests with Graal on and tier1-tier3 tests passed.
[1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8242239
Thanks,
Daniil
Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same
Hi Serguei and Chris,
Thank you for your comments.
I wanted to make the fix more generic and not limit it just to Graal
management bean. In this case we could expect that after the first retry is
triggered by Graal MBean registration some other “lazy-registered” MBean got
registered and the test might fail. To avoid this we need to allow test to make
at least several retry attempts before failing. If number 10 looks as too high
we could make it 5 or even 3. This should not affect the test run-time unless
the test starts failing for some other reason than we expect. In the new
webrev I will also correct the iteration counting (as Chris mentioned) and fix
typos.
Thanks again,
Daniil
From: "serguei.spit...@oracle.com"
Date: Friday, April 24, 2020 at 11:48 AM
To: Chris Plummer , Daniil Titov
, serviceability-dev
Subject: Re: RFR: 8242239: [Graal] javax/management/generified/GenericTest.java
fails: FAILED: queryMBeans sets same
Hi Daniil,
Besides what Chris is pointed out the fix looks good.
Minor:
97 while(true) {
113 if(mbeanCount * 2 == counterCount || retryCounter++ >
MAX_RETRY_ATTEMPT) {
114 assertEquals(mbeanCount * 2, counterCount);
Space is missed in while and if.
I doubt, the assert is really needed.
96 // is running ( e.g. Graal MBean). In this case just retry the
test.
Extra space before "e.g.".
Thanks,
Serguei
On 4/24/20 11:30, Chris Plummer wrote:
Hi Daniil,
84 // If new MBean (e.g. Graal MBean) is registred while the test
is running names1,
106 // If new MBean (e.g. Graal MBean) is registred while the test
is running mbeans1,
registred -> registered
',' after "running"
Just wondering how you chose 10 as the number of retries. Seems excessive.
Shouldn't the problem turn up at most 1 time and therefore only 1 retry is
needed.
76 int counter = 0;
86 if (sameSize(names1, names2, names3) || counter++ >
MAX_RETRY_ATTEMPTS) {
The way the checks are done you will actually end up retrying
MAX_RETRY_ATTEMPTS+1 times. For example, if MAX_RETRY_ATTEMPTS is 1, first time
through the loop counter is 0 so a retry is allowed. Second time through the
loop counter is 1, so a retry is allowed again.
thanks,
Chris
On 4/18/20 11:30 AM, Daniil Titov wrote:
Please review the change that fixes the failure of
javax/management/generified/GenericTest.java and
javax/management/query/CustomQueryTest.java tests when Graal is on.
The tests checks that calls of management API are consistent and return the
same set of MBeans. However,
when Graal is on the Graal MBean might be registered between the calls that in
turn makes the results
inconsistent and the tests fail.
The fix makes the test aware that some MBean might be registered while the
checks run and if it happens the tests repeat the check.
Testing : Mach5 tests with Graal on and tier1-tier3 tests passed.
[1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8242239
Thanks,
Daniil
Re: RFR: 8238561 serviceability/sa tests continue to run out of memory on Win* machines
Correction...
The option name is -showversion, not "-Xshowversion"
Best regards,
Daniil
On 4/23/20, 10:18 AM, "Daniil Titov" wrote:
Hi Chris,
I will revoke this RFR and resubmit it under JDK-8242009
with the changes you suggested to use Utils.getTestJavaOpts()
and make JDKToolLauncher to have an option to forward VM options.
> Is your change causing -Xshowversion to be passed?
Yes, the changes makes tests run with -Xshowversion to be passed.
> Do you know where it is coming from?
It is coming from task definitions for different tiers.
Thank you,
Daniil
On 4/22/20, 12:54 PM, "Chris Plummer" wrote:
Hi Daniil,
Thanks for cleaning this up. I think this should be fixed under
JDK-8242009. JDK-8238561 involves more than just this one issue.
Is there a reason why you didn't just change JDKToolLauncher to have an
option or API to add the args?
Why are you calling Utils.addTestJavaOpts() instead of
Utils.getTestJavaOpts()?
Is your change causing -Xshowversion to be passed? Do you know where it
is coming from?
thanks,
Chris
[1] https://bugs.openjdk.java.net/browse/JDK-8242009
On 4/22/20 10:48 AM, Daniil Titov wrote:
> Please review the change [1] that ensures that VM and test options
are forwarded to
> j*-tools when they are launched from serviceability/sa tests.
>
> In particular, it will ensure that passed to the tests maximum heap
size settings ( -XX:MaxRAMPercentage)
> are also honored by j*-tools serviceability/sa tests launch.
>
> The tests that expect an empty output were corrected to ignore the
product version printed
> in the error stream since in some tiers the tests are run with '
-showversion' VM option.
>
> Testing: Mach5 tests for tier1 - tier7 passed.
>
> [1] http://cr.openjdk.java.net/~dtitov/8238561/webrev.01
> [2] https://bugs.openjdk.java.net/browse/JDK-8238561
>
> Thank you,
> Daniil
>
>
Re: RFR: 8238561 serviceability/sa tests continue to run out of memory on Win* machines
Hi Chris, I will revoke this RFR and resubmit it under JDK-8242009 with the changes you suggested to use Utils.getTestJavaOpts() and make JDKToolLauncher to have an option to forward VM options. > Is your change causing -Xshowversion to be passed? Yes, the changes makes tests run with -Xshowversion to be passed. > Do you know where it is coming from? It is coming from task definitions for different tiers. Thank you, Daniil On 4/22/20, 12:54 PM, "Chris Plummer" wrote: Hi Daniil, Thanks for cleaning this up. I think this should be fixed under JDK-8242009. JDK-8238561 involves more than just this one issue. Is there a reason why you didn't just change JDKToolLauncher to have an option or API to add the args? Why are you calling Utils.addTestJavaOpts() instead of Utils.getTestJavaOpts()? Is your change causing -Xshowversion to be passed? Do you know where it is coming from? thanks, Chris [1] https://bugs.openjdk.java.net/browse/JDK-8242009 On 4/22/20 10:48 AM, Daniil Titov wrote: > Please review the change [1] that ensures that VM and test options are forwarded to > j*-tools when they are launched from serviceability/sa tests. > > In particular, it will ensure that passed to the tests maximum heap size settings ( -XX:MaxRAMPercentage) > are also honored by j*-tools serviceability/sa tests launch. > > The tests that expect an empty output were corrected to ignore the product version printed > in the error stream since in some tiers the tests are run with ' -showversion' VM option. > > Testing: Mach5 tests for tier1 - tier7 passed. > > [1] http://cr.openjdk.java.net/~dtitov/8238561/webrev.01 > [2] https://bugs.openjdk.java.net/browse/JDK-8238561 > > Thank you, > Daniil > >
RFR: 8238561 serviceability/sa tests continue to run out of memory on Win* machines
Please review the change [1] that ensures that VM and test options are forwarded to j*-tools when they are launched from serviceability/sa tests. In particular, it will ensure that passed to the tests maximum heap size settings ( -XX:MaxRAMPercentage) are also honored by j*-tools serviceability/sa tests launch. The tests that expect an empty output were corrected to ignore the product version printed in the error stream since in some tiers the tests are run with ' -showversion' VM option. Testing: Mach5 tests for tier1 - tier7 passed. [1] http://cr.openjdk.java.net/~dtitov/8238561/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8238561 Thank you, Daniil
Re: 8231585: java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java fails with java.lang.NullPointerException
Hi David, Serguei, and Chris, Thank you for reviewing this change. The issue is not easy to reproduce. I tried to run the test (without the fix) with Graal on in Mach5 about 600 times and so far the failure didn't reproduce. I will continue running these tests in the next few days and will update the issue with the information about disappearing thread when it gets reproduced. Best regards, Daniil On 4/18/20, 6:56 AM, "David Holmes" wrote: Hi Daniil, On 18/04/2020 6:03 am, Daniil Titov wrote: > Please review the change that fixes intermittent failure of java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java > > As David noticed (thank you, David, for this analysis) there is no guarantee that all threads found by getAllThreadIds() are still alive by the time we call getThreadInfo() so we have to allow for null array entries. > > Testing: Mach5 tests with Graal on passed 300 times. Fix looks good - thanks. I think it would be informative to actually determine which thread(s) is disappearing (not as part of the test just as some diagnostic info to add to the JBS issue). Can you reproduce the failure easily? Thanks, David > [1] http://cr.openjdk.java.net/~dtitov/8231585/webrev.01/ > [2] https://bugs.openjdk.java.net/browse/JDK-8231585 > > Best regards, > Daniil > >
RFR: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same
Please review the change that fixes the failure of javax/management/generified/GenericTest.java and javax/management/query/CustomQueryTest.java tests when Graal is on. The tests checks that calls of management API are consistent and return the same set of MBeans. However, when Graal is on the Graal MBean might be registered between the calls that in turn makes the results inconsistent and the tests fail. The fix makes the test aware that some MBean might be registered while the checks run and if it happens the tests repeat the check. Testing : Mach5 tests with Graal on and tier1-tier3 tests passed. [1] http://cr.openjdk.java.net/~dtitov/8242239/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8242239 Thanks, Daniil
RFR: 8231585: java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java fails with java.lang.NullPointerException
Please review the change that fixes intermittent failure of java/lang/management/ThreadMXBean/MaxDepthForThreadInfoTest.java As David noticed (thank you, David, for this analysis) there is no guarantee that all threads found by getAllThreadIds() are still alive by the time we call getThreadInfo() so we have to allow for null array entries. Testing: Mach5 tests with Graal on passed 300 times. [1] http://cr.openjdk.java.net/~dtitov/8231585/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8231585 Best regards, Daniil
RFR 8242430: Correct links in javadoc of OperatingSystemMXBean
Please review a javadoc fix [1] that corrects the links in the description of getTotalPhysicalMemorySize() method in com.sun.management. OperatingSystemMXBean class. The CSR [2] needs a reviewer as well. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8242430/webrev.01/ [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8242431 [3] Bug: https://bugs.openjdk.java.net/browse/JDK-8242430 Thank you, Daniil
Re: 8241530: com/sun/jdi tests fail due to network issues on OSX 10.15
Thank you, Serguei and Alex, for reviewing this change. Best regards, Daniil On 4/1/20, 2:19 PM, "serguei.spit...@oracle.com" wrote: Hi Daniil, LGTM++ Thanks, Serguei On 3/30/20 13:06, Alex Menkov wrote: > Looks good. > > --alex > > On 03/30/2020 12:43, Daniil Titov wrote: >> Please review the change [1] that fixes the failure of >> com/sun/jdi/JdwpListenTest.java >> and com/sun/jdi/JdwpAttachTest.java tests on OSX 10.15. >> >> The problem here is the similar to the one solved in [4] by >> additional filtering >> of unusual network interfaces in the test library class >> jdk.test.lib.NetworkConfiguration. >> However, the failing com/sun/jdi tests do not use >> jdk.test.lib.NetworkConfiguration and >> Instead do repeat the same logic themselves. >> >> The fix changes these tests to start using >> jdk.test.lib.NetworkConfiguration to find all local addresses. >> >> Initially the issue [2] also included 3 other failing tests from >> sun/management/jdp package, but these tests fail >> for a different reason so I moved them in the new issue [3] and >> updated the ProblemList.txt for them. >> >> >> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8241530/webrev.01/ >> [2] Jira Issue: https://bugs.openjdk.java.net/browse/JDK-8241530 >> [3] https://bugs.openjdk.java.net/browse/JDK-8241865 >> [4] https://bugs.openjdk.java.net/browse/JDK-8241336 >> >> Thank you, >> Daniil >> >>
RFR: 8241530: com/sun/jdi tests fail due to network issues on OSX 10.15
Please review the change [1] that fixes the failure of com/sun/jdi/JdwpListenTest.java and com/sun/jdi/JdwpAttachTest.java tests on OSX 10.15. The problem here is the similar to the one solved in [4] by additional filtering of unusual network interfaces in the test library class jdk.test.lib.NetworkConfiguration. However, the failing com/sun/jdi tests do not use jdk.test.lib.NetworkConfiguration and Instead do repeat the same logic themselves. The fix changes these tests to start using jdk.test.lib.NetworkConfiguration to find all local addresses. Initially the issue [2] also included 3 other failing tests from sun/management/jdp package, but these tests fail for a different reason so I moved them in the new issue [3] and updated the ProblemList.txt for them. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8241530/webrev.01/ [2] Jira Issue: https://bugs.openjdk.java.net/browse/JDK-8241530 [3] https://bugs.openjdk.java.net/browse/JDK-8241865 [4] https://bugs.openjdk.java.net/browse/JDK-8241336 Thank you, Daniil
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa and Serguei,
Thank you for reviewing this change.
Best regards,
--Daniil
On 3/25/20, 1:01 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
On 3/24/20 10:00, Daniil Titov wrote:
> Hi Serguei,
>
>> It looks like you removed the last call site of DebugServer.main.
> Yes. It is correct.
>
>> Do we need to remove the DebugServer.java as well?
> I was considering this but since it is a public class I think it needs to
be deprecated first. I also think that it would be better to do in a separate
issue
> since a CSR for deprecation needs to be filed for that. If you agree I
will create a new issue for that.
I'm okay to separate this.
Thanks,
Serguei
>
> Thanks,
> Daniil
>
>
> On 3/23/20, 11:56 PM, "serguei.spit...@oracle.com"
wrote:
>
> Hi Daniil,
>
> It looks pretty good in general.
>
> It looks like you removed the last call site of DebugServer.main.
> Do we need to remove the DebugServer.java as well?
>
> Thanks,
> Serguei
>
>
> On 3/22/20 15:29, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the webrev that merges
SADebugDTest.java with changes done in [2].
> >
> > Also the CRS [3] and the help message for debug server in
SALauncher.java were updated to specify that '--hostname'
> > option could be a hostname or an IPv4/IPv6 address.
> >
> > > Ok, but I think it might be more simply with TestLibrary.
> > > For example, can you use TestLibrary::getUnusedRandomPort ?
It is used in test/jdk/java/rmi/testlibrary/RMID.java .
> >
> > TestLibrary:: getUnusedRandomPort() doesn't allow to specify what
ports are reserved and it uses some hardcoded port range [FIXED_PORT_MIN,
FIXED_PORT_MAX] as reserved ports. Besides,
test/jdk/java/rmi/testlibrary/TestLibrary.java class cannot be directly used in
test/hotspot/jtreg/serviceability/* tests (it doesn't compile).
> >
> > Nevertheless, to simplify the test itself I moved
findUnreservedFreePort(int .. reservedPorts) from SADebugTest.java to
jdk.test.lib.Utils in /test/lib.
> >
> > Testing: Mach5 tier1-tier3 tests (that include
serviceability/sa/sadebugd tests) succeeded.
> >
> > [1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
> > [2] https://bugs.openjdk.java.net/browse/JDK-8238268
> > [3] https://bugs.openjdk.java.net/browse/JDK-8239831
> >
> > Thank you,
> > Daniil
> >
> > On 3/13/20, 7:23 PM, "Yasumasa Suenaga"
wrote:
> >
> > Hi Daniil,
> >
> > On 2020/03/14 7:05, Daniil Titov wrote:
> > > Hi Yasumasa, Serguei and Alex,
> > >
> > > Please review a new version of the webrev that includes the
changes Yasumasa suggested.
> > >
> > >> Shutdown hook is already registered in c'tor of
HotSpotAgent.
> > >> It works same as shutdownServer(), so I think shutdown
hook at SALauncher is not needed.
> > >
> > > The shutdown hook registered in the HotSpotAgent c'tor only
works for non-servers, so we still need a
> > > the shutdown hook for remote server being added in
SALauncher. I changed it to use the lambda expression.
> > >
> > > 101 public HotSpotAgent() {
> > > 102 // for non-server add shutdown hook to
clean-up debugger in case
> > > 103 // of forced exit. For remote server,
shutdown hook is added by
> > > 104 // DebugServer.
> > > 105 Runtime.getRuntime().addShutdownHook(new
java.lang.Thread(
> > > 106 new Runnable() {
> > > 107 public void run() {
> > > 108 synchronized (HotSpotAgent.this) {
> > > 109 if (!isServer) {
> > > 110 detach();
> > > 111 }
> > > 112 }
> > > 113 }
>
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Serguei,
>It looks like you removed the last call site of DebugServer.main.
Yes. It is correct.
>Do we need to remove the DebugServer.java as well?
I was considering this but since it is a public class I think it needs to be
deprecated first. I also think that it would be better to do in a separate issue
since a CSR for deprecation needs to be filed for that. If you agree I will
create a new issue for that.
Thanks,
Daniil
On 3/23/20, 11:56 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
It looks pretty good in general.
It looks like you removed the last call site of DebugServer.main.
Do we need to remove the DebugServer.java as well?
Thanks,
Serguei
On 3/22/20 15:29, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
>
> Please review a new version of the webrev that merges SADebugDTest.java
with changes done in [2].
>
> Also the CRS [3] and the help message for debug server in SALauncher.java
were updated to specify that '--hostname'
> option could be a hostname or an IPv4/IPv6 address.
>
> > Ok, but I think it might be more simply with TestLibrary.
> > For example, can you use TestLibrary::getUnusedRandomPort ? It is
used in test/jdk/java/rmi/testlibrary/RMID.java .
>
> TestLibrary:: getUnusedRandomPort() doesn't allow to specify what ports
are reserved and it uses some hardcoded port range [FIXED_PORT_MIN,
FIXED_PORT_MAX] as reserved ports. Besides,
test/jdk/java/rmi/testlibrary/TestLibrary.java class cannot be directly used in
test/hotspot/jtreg/serviceability/* tests (it doesn't compile).
>
> Nevertheless, to simplify the test itself I moved
findUnreservedFreePort(int .. reservedPorts) from SADebugTest.java to
jdk.test.lib.Utils in /test/lib.
>
> Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
>
> [1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
> [2] https://bugs.openjdk.java.net/browse/JDK-8238268
> [3] https://bugs.openjdk.java.net/browse/JDK-8239831
>
> Thank you,
> Daniil
>
> On 3/13/20, 7:23 PM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
> On 2020/03/14 7:05, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the webrev that includes the
changes Yasumasa suggested.
> >
> >> Shutdown hook is already registered in c'tor of HotSpotAgent.
> >> It works same as shutdownServer(), so I think shutdown hook
at SALauncher is not needed.
> >
> > The shutdown hook registered in the HotSpotAgent c'tor only works
for non-servers, so we still need a
> > the shutdown hook for remote server being added in SALauncher. I
changed it to use the lambda expression.
> >
> > 101 public HotSpotAgent() {
> > 102 // for non-server add shutdown hook to clean-up
debugger in case
> > 103 // of forced exit. For remote server, shutdown hook
is added by
> > 104 // DebugServer.
> > 105 Runtime.getRuntime().addShutdownHook(new
java.lang.Thread(
> > 106 new Runnable() {
> > 107 public void run() {
> > 108 synchronized (HotSpotAgent.this) {
> > 109 if (!isServer) {
> > 110 detach();
> > 111 }
> > 112 }
> > 113 }
> > 114 }));
> > 115 }
>
> I missed it, thanks!
>
>
> >>> Hmm... I think port check (already in use) is not needed
because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
> >>> `exclusiveAccess.dirs=.` to avoid concurrent execution
> > As I understand exclusiveAccess.dirs prevents only the tests
located in this directory from being run simultaneously and other tests could
still run in parallel with one of these tests. Thus I would prefer to have the
retry mechanism in place. I simplified the code using the class variables
instead of local arrays.
>
> Ok, but I think it might be more simply with TestLibrary.
> For example, can you use TestLibrary::getUnusedRandomPort ? It is
used in test/jdk/java/rmi/testlibrary/RMID.java .
>
>
> Thanks,
>
> Yasumasa
>
Re: RFR: 8240711: TestJstatdPort.java failed due to "ExportException: Port already in use:"
Hi Serguei,
I don’t think that in any real environment the loop could not be able to find
the pair of free ports before it is killed by JTREG due to timeout. But if you
think that we need to limit the number of attempts here I could create a new
issue for that.
Thanks!
--Daniil
From: "serguei.spit...@oracle.com"
Date: Monday, March 23, 2020 at 12:13 PM
To: Daniil Titov , Alex Menkov
, serviceability-dev
Subject: Re: 8240711: TestJstatdPort.java failed due to "ExportException: Port
already in use:"
On 3/23/20 12:05, Daniil Titov wrote:
Hi Serguei,
In this case tryToSetupJstatdProcess() on line 346 return null and the test
will try to find a new pair of ports and start jstatd process.
I understand this.
My question if this loop can be endless.
What happens if there is no new pair of ports that we did not check yet?
Do we fail with a timeout in such a case?
If so, would it better to report that unused free port was not found?
Is it possible to detect this situation?
Thanks,
Serguei
Best regards,
Daniil
From: "serguei.spit...@oracle.com"
Date: Monday, March 23, 2020 at 11:45 AM
To: Daniil Titov , Alex Menkov
, serviceability-dev
Subject: Re: RFR: 8240711: TestJstatdPort.java failed due to "ExportException:
Port already in use:"
Hi Daniil,
It looks Okay in general.
But I've got a question.
329 while (jstatdThread == null) {
330 if (!useDefaultPort) {
331 port = String.valueOf(Utils.getFreePort());
332 }
333
334 if (!useDefaultRmiPort) {
335 rmiPort = String.valueOf(Utils.getFreePort());
336 }
337
338 if (withExternalRegistry) {
339 Registry registry = startRegistry();
340 if (registry == null) {
341 // The port is already in use. Cancel and try with
a new one.
342 continue;
343 }
344 }
345
346 jstatdThread = tryToSetupJstatdProcess();
347 }
What is going to happen if all ports that we try are already in use?
Does the test report this situation?
Thanks,
Serguei
On 3/17/20 11:40, Daniil Titov wrote:
Hi Alex,
Please review a new version of the fix that removes the old version of the code
that tried to handle the "port in use" case.
Testing: Mach5 tests for sun/tools/jstatd/ successfully passed 100 times.
Tier1-tier3 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.02
[2] https://bugs.openjdk.java.net/browse/JDK-8240711
Thanks,
Daniil
On 3/16/20, 5:38 PM, "Daniil Titov" wrote:
Hi Alex,
Yes, I did test the change by modifying the test to use the RMI port that
is already in use
( the stack trace in the original email was exact from this changed test)
and then ensured that with the fix
the such issue is properly handled.
I will send a new version of the webrev that removes the old version of the
code that tried to handle the "port in use" case.
Thanks!
Best regards,
Daniil
On 3/16/20, 4:47 PM, "Alex Menkov" wrote:
I don't agree.
The code handles exact the same "port in use" case for the same tool.
So it either works or doesn't.
And have 2 code blocks which suppose to do the same makes the code
messy.
BTW did you tested the change (I mean craft the test to get "port in
use" error)?
--alex
On 03/16/2020 16:17, Daniil Titov wrote:
> Resending with the corrected subject ...
>
> Hi Alex,
>
> Yes, you are right, class JstatdTest has the code that is supposed to
handle the "port in use"
> case but at least for this specific test
(sun/tools/jstatd/TestJstatdPort.java) it doesn't work.
>
> Since there are multiple tests in sun/tools/jstatd/* folder that use
this class and different ports
> might be subject to the "port in use" error and taking into account
that it's hard to reproduce such case
> I found it safer to leave the original code and just augment it with
what was missing for this specific
> case rather than completely replacing it.
>
> Best regards,
> Daniil
>
> On 3/16/20, 4:02 PM, "Alex Menkov" wrote:
>
> Hi Daniil,
>
> Looks like the test is supposed to handle "port in use" issue
(see lines
> 103-114).
> I suppose in case "port in use" jstatd exits, but
> ProcessTools.startProce
Re: 8240711: TestJstatdPort.java failed due to "ExportException: Port already in use:"
Hi Serguei,
In this case tryToSetupJstatdProcess() on line 346 return null and the test
will try to find a new pair of ports and start jstatd process.
Best regards,
Daniil
From: "serguei.spit...@oracle.com"
Date: Monday, March 23, 2020 at 11:45 AM
To: Daniil Titov , Alex Menkov
, serviceability-dev
Subject: Re: RFR: 8240711: TestJstatdPort.java failed due to "ExportException:
Port already in use:"
Hi Daniil,
It looks Okay in general.
But I've got a question.
329 while (jstatdThread == null) {
330 if (!useDefaultPort) {
331 port = String.valueOf(Utils.getFreePort());
332 }
333
334 if (!useDefaultRmiPort) {
335 rmiPort = String.valueOf(Utils.getFreePort());
336 }
337
338 if (withExternalRegistry) {
339 Registry registry = startRegistry();
340 if (registry == null) {
341 // The port is already in use. Cancel and try with
a new one.
342 continue;
343 }
344 }
345
346 jstatdThread = tryToSetupJstatdProcess();
347 }
What is going to happen if all ports that we try are already in use?
Does the test report this situation?
Thanks,
Serguei
On 3/17/20 11:40, Daniil Titov wrote:
Hi Alex,
Please review a new version of the fix that removes the old version of the code
that tried to handle the "port in use" case.
Testing: Mach5 tests for sun/tools/jstatd/ successfully passed 100 times.
Tier1-tier3 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.02
[2] https://bugs.openjdk.java.net/browse/JDK-8240711
Thanks,
Daniil
On 3/16/20, 5:38 PM, "Daniil Titov" wrote:
Hi Alex,
Yes, I did test the change by modifying the test to use the RMI port that
is already in use
( the stack trace in the original email was exact from this changed test)
and then ensured that with the fix
the such issue is properly handled.
I will send a new version of the webrev that removes the old version of the
code that tried to handle the "port in use" case.
Thanks!
Best regards,
Daniil
On 3/16/20, 4:47 PM, "Alex Menkov" wrote:
I don't agree.
The code handles exact the same "port in use" case for the same tool.
So it either works or doesn't.
And have 2 code blocks which suppose to do the same makes the code
messy.
BTW did you tested the change (I mean craft the test to get "port in
use" error)?
--alex
On 03/16/2020 16:17, Daniil Titov wrote:
> Resending with the corrected subject ...
>
> Hi Alex,
>
> Yes, you are right, class JstatdTest has the code that is supposed to
handle the "port in use"
> case but at least for this specific test
(sun/tools/jstatd/TestJstatdPort.java) it doesn't work.
>
> Since there are multiple tests in sun/tools/jstatd/* folder that use
this class and different ports
> might be subject to the "port in use" error and taking into account
that it's hard to reproduce such case
> I found it safer to leave the original code and just augment it with
what was missing for this specific
> case rather than completely replacing it.
>
> Best regards,
> Daniil
>
> On 3/16/20, 4:02 PM, "Alex Menkov" wrote:
>
> Hi Daniil,
>
> Looks like the test is supposed to handle "port in use" issue
(see lines
> 103-114).
> I suppose in case "port in use" jstatd exits, but
> ProcessTools.startProcess() continue to wait for "jstatd
started" message.
>
> --alex
>
> On 03/16/2020 12:00, Daniil Titov wrote:
> > Please review the change [1] that fixes the intermittent
failure of the test.
> >
> > The problem here is that if the RMI port is in use than the
test keep waiting for "jstatd started (bound to " to appear in the process
output and in this case
> > It doesn't happen.
> >
> > at
java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232)
> > at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205)
> > at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:13
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa, Serguei and Alex,
Please review a new version of the webrev that merges SADebugDTest.java with
changes done in [2].
Also the CRS [3] and the help message for debug server in SALauncher.java were
updated to specify that '--hostname'
option could be a hostname or an IPv4/IPv6 address.
> Ok, but I think it might be more simply with TestLibrary.
> For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
> test/jdk/java/rmi/testlibrary/RMID.java .
TestLibrary:: getUnusedRandomPort() doesn't allow to specify what ports are
reserved and it uses some hardcoded port range [FIXED_PORT_MIN, FIXED_PORT_MAX]
as reserved ports. Besides, test/jdk/java/rmi/testlibrary/TestLibrary.java
class cannot be directly used in test/hotspot/jtreg/serviceability/* tests (it
doesn't compile).
Nevertheless, to simplify the test itself I moved findUnreservedFreePort(int ..
reservedPorts) from SADebugTest.java to jdk.test.lib.Utils in /test/lib.
Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
[1] http://cr.openjdk.java.net/~dtitov/8196751/webrev.04/
[2] https://bugs.openjdk.java.net/browse/JDK-8238268
[3] https://bugs.openjdk.java.net/browse/JDK-8239831
Thank you,
Daniil
On 3/13/20, 7:23 PM, "Yasumasa Suenaga" wrote:
Hi Daniil,
On 2020/03/14 7:05, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
>
> Please review a new version of the webrev that includes the changes
Yasumasa suggested.
>
>> Shutdown hook is already registered in c'tor of HotSpotAgent.
>> It works same as shutdownServer(), so I think shutdown hook at
SALauncher is not needed.
>
> The shutdown hook registered in the HotSpotAgent c'tor only works for
non-servers, so we still need a
> the shutdown hook for remote server being added in SALauncher. I changed
it to use the lambda expression.
>
> 101 public HotSpotAgent() {
> 102 // for non-server add shutdown hook to clean-up debugger in
case
> 103 // of forced exit. For remote server, shutdown hook is
added by
> 104 // DebugServer.
> 105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
> 106 new Runnable() {
> 107 public void run() {
> 108 synchronized (HotSpotAgent.this) {
> 109 if (!isServer) {
> 110 detach();
> 111 }
> 112 }
> 113 }
> 114 }));
> 115 }
I missed it, thanks!
>>> Hmm... I think port check (already in use) is not needed because
test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
>>> `exclusiveAccess.dirs=.` to avoid concurrent execution
> As I understand exclusiveAccess.dirs prevents only the tests located in
this directory from being run simultaneously and other tests could still run in
parallel with one of these tests. Thus I would prefer to have the retry
mechanism in place. I simplified the code using the class variables instead of
local arrays.
Ok, but I think it might be more simply with TestLibrary.
For example, can you use TestLibrary::getUnusedRandomPort ? It is used in
test/jdk/java/rmi/testlibrary/RMID.java .
Thanks,
Yasumasa
> Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
> [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
> [3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751
>
> Thank you,
> Daniil
>
> On 3/6/20, 6:15 PM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
> On 2020/03/07 3:38, Daniil Titov wrote:
> > Hi Yasumasa,
> >
> > -> checkBasicOptions() is needed? I think you can remove this
method and embed it in caller.
> > I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect (
it throws an exception if parameters are incorrect) and ignores its return
might look confusing. Thus, I found it more appropriate to wrap it inside a
method with more relevant name .
>
> Ok, but I prefer to leave comment it.
>
>
> > > SADebugDTest
> > > - Why do you declare portInUse and testResult as array? Their
length is 1, so I think you don't need to use array.
> > We cannot use primitives there since these local va
Re: RFR: 8240711: TestJstatdPort.java failed due to "ExportException: Port already in use:"
Hi Alex,
Thank you for reviewing this change.
Best regards,
Daniil
On 3/17/20, 11:58 AM, "Alex Menkov" wrote:
LGTM
--alex
On 03/17/2020 11:40, Daniil Titov wrote:
> Hi Alex,
>
> Please review a new version of the fix that removes the old version of
the code that tried to handle the "port in use" case.
>
> Testing: Mach5 tests for sun/tools/jstatd/ successfully passed 100
times. Tier1-tier3 tests successfully passed.
>
> [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.02
> [2] https://bugs.openjdk.java.net/browse/JDK-8240711
>
> Thanks,
> Daniil
>
>
>
> On 3/16/20, 5:38 PM, "Daniil Titov" wrote:
>
> Hi Alex,
>
> Yes, I did test the change by modifying the test to use the RMI
port that is already in use
> ( the stack trace in the original email was exact from this changed
test) and then ensured that with the fix
> the such issue is properly handled.
>
> I will send a new version of the webrev that removes the old version
of the code that tried to handle the "port in use" case.
>
> Thanks!
>
> Best regards,
> Daniil
>
>
>
>
> On 3/16/20, 4:47 PM, "Alex Menkov" wrote:
>
> I don't agree.
> The code handles exact the same "port in use" case for the same
tool.
> So it either works or doesn't.
> And have 2 code blocks which suppose to do the same makes the
code messy.
> BTW did you tested the change (I mean craft the test to get
"port in
> use" error)?
>
> --alex
>
> On 03/16/2020 16:17, Daniil Titov wrote:
> > Resending with the corrected subject ...
> >
> > Hi Alex,
> >
> > Yes, you are right, class JstatdTest has the code that is
supposed to handle the "port in use"
> > case but at least for this specific test
(sun/tools/jstatd/TestJstatdPort.java) it doesn't work.
> >
> > Since there are multiple tests in sun/tools/jstatd/* folder
that use this class and different ports
> > might be subject to the "port in use" error and taking into
account that it's hard to reproduce such case
> > I found it safer to leave the original code and just augment
it with what was missing for this specific
> > case rather than completely replacing it.
> >
> > Best regards,
> > Daniil
> >
> > On 3/16/20, 4:02 PM, "Alex Menkov"
wrote:
> >
> > Hi Daniil,
> >
> > Looks like the test is supposed to handle "port in use"
issue (see lines
> > 103-114).
> > I suppose in case "port in use" jstatd exits, but
> > ProcessTools.startProcess() continue to wait for "jstatd
started" message.
> >
> > --alex
> >
> > On 03/16/2020 12:00, Daniil Titov wrote:
> > > Please review the change [1] that fixes the
intermittent failure of the test.
> > >
> > > The problem here is that if the RMI port is in use than
the test keep waiting for "jstatd started (bound to " to appear in the process
output and in this case
> > > It doesn't happen.
> > >
> > > at
java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232)
> > > at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205)
> > > at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:133)
> > > at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:254)
> > > at
jdk.test.lib.thread.ProcessThread$ProcessRunnable.xrun(ProcessThread.java:153)
> > > at jdk.test.lib.thread.XRun.run(XRun.java:40)
> > > at
java.lang.Thread.run(java.base@15-internal/Thread.java:832)
> > > at
jdk.test.lib.thread.TestThread.run(TestThread.java:123)
> > >
> > > Testing: Mach5 tests for sun/tools/jstatd/ successfully
passed. Tier1-tier3 tests are still in progress.
> > >
> > > [1]
http://cr.openjdk.java.net/~dtitov/8240711/webrev.01/
> > > [2] https://bugs.openjdk.java.net/browse/JDK-8240711
> > >
> > >
> > > Thank you,
> > > Daniil
> > >
> > >
> > >
> >
> >
> >
>
>
>
>
Re: RFR: 8240711: TestJstatdPort.java failed due to "ExportException: Port already in use:"
Hi Alex,
Please review a new version of the fix that removes the old version of the code
that tried to handle the "port in use" case.
Testing: Mach5 tests for sun/tools/jstatd/ successfully passed 100 times.
Tier1-tier3 tests successfully passed.
[1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.02
[2] https://bugs.openjdk.java.net/browse/JDK-8240711
Thanks,
Daniil
On 3/16/20, 5:38 PM, "Daniil Titov" wrote:
Hi Alex,
Yes, I did test the change by modifying the test to use the RMI port that
is already in use
( the stack trace in the original email was exact from this changed test)
and then ensured that with the fix
the such issue is properly handled.
I will send a new version of the webrev that removes the old version of the
code that tried to handle the "port in use" case.
Thanks!
Best regards,
Daniil
On 3/16/20, 4:47 PM, "Alex Menkov" wrote:
I don't agree.
The code handles exact the same "port in use" case for the same tool.
So it either works or doesn't.
And have 2 code blocks which suppose to do the same makes the code
messy.
BTW did you tested the change (I mean craft the test to get "port in
use" error)?
--alex
On 03/16/2020 16:17, Daniil Titov wrote:
> Resending with the corrected subject ...
>
> Hi Alex,
>
> Yes, you are right, class JstatdTest has the code that is supposed to
handle the "port in use"
> case but at least for this specific test
(sun/tools/jstatd/TestJstatdPort.java) it doesn't work.
>
> Since there are multiple tests in sun/tools/jstatd/* folder that use
this class and different ports
> might be subject to the "port in use" error and taking into account
that it's hard to reproduce such case
> I found it safer to leave the original code and just augment it with
what was missing for this specific
> case rather than completely replacing it.
>
> Best regards,
> Daniil
>
> On 3/16/20, 4:02 PM, "Alex Menkov" wrote:
>
> Hi Daniil,
>
> Looks like the test is supposed to handle "port in use" issue
(see lines
> 103-114).
> I suppose in case "port in use" jstatd exits, but
> ProcessTools.startProcess() continue to wait for "jstatd
started" message.
>
> --alex
>
> On 03/16/2020 12:00, Daniil Titov wrote:
> > Please review the change [1] that fixes the intermittent
failure of the test.
> >
> > The problem here is that if the RMI port is in use than the
test keep waiting for "jstatd started (bound to " to appear in the process
output and in this case
> > It doesn't happen.
> >
> >at
java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232)
> >at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205)
> >at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:133)
> >at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:254)
> >at
jdk.test.lib.thread.ProcessThread$ProcessRunnable.xrun(ProcessThread.java:153)
> >at jdk.test.lib.thread.XRun.run(XRun.java:40)
> >at
java.lang.Thread.run(java.base@15-internal/Thread.java:832)
> >at
jdk.test.lib.thread.TestThread.run(TestThread.java:123)
> >
> > Testing: Mach5 tests for sun/tools/jstatd/ successfully
passed. Tier1-tier3 tests are still in progress.
> >
> > [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.01/
> > [2] https://bugs.openjdk.java.net/browse/JDK-8240711
> >
> >
> > Thank you,
> > Daniil
> >
> >
> >
>
>
>
Re: RFR: 8240711: TestJstatdPort.java failed due to "ExportException: Port already in use:"
Hi Alex,
Yes, I did test the change by modifying the test to use the RMI port that is
already in use
( the stack trace in the original email was exact from this changed test) and
then ensured that with the fix
the such issue is properly handled.
I will send a new version of the webrev that removes the old version of the
code that tried to handle the "port in use" case.
Thanks!
Best regards,
Daniil
On 3/16/20, 4:47 PM, "Alex Menkov" wrote:
I don't agree.
The code handles exact the same "port in use" case for the same tool.
So it either works or doesn't.
And have 2 code blocks which suppose to do the same makes the code messy.
BTW did you tested the change (I mean craft the test to get "port in
use" error)?
--alex
On 03/16/2020 16:17, Daniil Titov wrote:
> Resending with the corrected subject ...
>
> Hi Alex,
>
> Yes, you are right, class JstatdTest has the code that is supposed to
handle the "port in use"
> case but at least for this specific test
(sun/tools/jstatd/TestJstatdPort.java) it doesn't work.
>
> Since there are multiple tests in sun/tools/jstatd/* folder that use this
class and different ports
> might be subject to the "port in use" error and taking into account that
it's hard to reproduce such case
> I found it safer to leave the original code and just augment it with what
was missing for this specific
> case rather than completely replacing it.
>
> Best regards,
> Daniil
>
> On 3/16/20, 4:02 PM, "Alex Menkov" wrote:
>
> Hi Daniil,
>
> Looks like the test is supposed to handle "port in use" issue (see
lines
> 103-114).
> I suppose in case "port in use" jstatd exits, but
> ProcessTools.startProcess() continue to wait for "jstatd started"
message.
>
> --alex
>
> On 03/16/2020 12:00, Daniil Titov wrote:
> > Please review the change [1] that fixes the intermittent failure
of the test.
> >
> > The problem here is that if the RMI port is in use than the test
keep waiting for "jstatd started (bound to " to appear in the process output
and in this case
> > It doesn't happen.
> >
> >at
java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232)
> >at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205)
> >at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:133)
> >at
jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:254)
> >at
jdk.test.lib.thread.ProcessThread$ProcessRunnable.xrun(ProcessThread.java:153)
> >at jdk.test.lib.thread.XRun.run(XRun.java:40)
> >at java.lang.Thread.run(java.base@15-internal/Thread.java:832)
> >at jdk.test.lib.thread.TestThread.run(TestThread.java:123)
> >
> > Testing: Mach5 tests for sun/tools/jstatd/ successfully passed.
Tier1-tier3 tests are still in progress.
> >
> > [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.01/
> > [2] https://bugs.openjdk.java.net/browse/JDK-8240711
> >
> >
> > Thank you,
> > Daniil
> >
> >
> >
>
>
>
Re: RFR: 8240711: TestJstatdPort.java failed due to "ExportException: Port already in use:"
Resending with the corrected subject ... Hi Alex, Yes, you are right, class JstatdTest has the code that is supposed to handle the "port in use" case but at least for this specific test (sun/tools/jstatd/TestJstatdPort.java) it doesn't work. Since there are multiple tests in sun/tools/jstatd/* folder that use this class and different ports might be subject to the "port in use" error and taking into account that it's hard to reproduce such case I found it safer to leave the original code and just augment it with what was missing for this specific case rather than completely replacing it. Best regards, Daniil On 3/16/20, 4:02 PM, "Alex Menkov" wrote: Hi Daniil, Looks like the test is supposed to handle "port in use" issue (see lines 103-114). I suppose in case "port in use" jstatd exits, but ProcessTools.startProcess() continue to wait for "jstatd started" message. --alex On 03/16/2020 12:00, Daniil Titov wrote: > Please review the change [1] that fixes the intermittent failure of the test. > > The problem here is that if the RMI port is in use than the test keep waiting for "jstatd started (bound to " to appear in the process output and in this case > It doesn't happen. > > at java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232) > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205) > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:133) > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:254) > at jdk.test.lib.thread.ProcessThread$ProcessRunnable.xrun(ProcessThread.java:153) > at jdk.test.lib.thread.XRun.run(XRun.java:40) > at java.lang.Thread.run(java.base@15-internal/Thread.java:832) > at jdk.test.lib.thread.TestThread.run(TestThread.java:123) > > Testing: Mach5 tests for sun/tools/jstatd/ successfully passed. Tier1-tier3 tests are still in progress. > > [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.01/ > [2] https://bugs.openjdk.java.net/browse/JDK-8240711 > > > Thank you, > Daniil > > >
Re: 8240711: TestJstatdPort.java failed due to "ExportException: Port already in use:"
Hi Alex, Yes, you are right, class JstatdTest has the code that is supposed to handle the "port in use" case but at least for this specific test sun/tools/jstatd/TestJstatdPort.java is doesn't work. Since there are multiple tests in sun/tools/jstatd/* folder that use this class and different ports might be subject to the "port in use" error and taking into account that it's hard to reproduce such case I found it safer to leave the original code and just augment it with what was missing for this specific case rather than completely replacing it. Best regards, Daniil On 3/16/20, 4:02 PM, "Alex Menkov" wrote: Hi Daniil, Looks like the test is supposed to handle "port in use" issue (see lines 103-114). I suppose in case "port in use" jstatd exits, but ProcessTools.startProcess() continue to wait for "jstatd started" message. --alex On 03/16/2020 12:00, Daniil Titov wrote: > Please review the change [1] that fixes the intermittent failure of the test. > > The problem here is that if the RMI port is in use than the test keep waiting for "jstatd started (bound to " to appear in the process output and in this case > It doesn't happen. > > at java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232) > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205) > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:133) > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:254) > at jdk.test.lib.thread.ProcessThread$ProcessRunnable.xrun(ProcessThread.java:153) > at jdk.test.lib.thread.XRun.run(XRun.java:40) > at java.lang.Thread.run(java.base@15-internal/Thread.java:832) > at jdk.test.lib.thread.TestThread.run(TestThread.java:123) > > Testing: Mach5 tests for sun/tools/jstatd/ successfully passed. Tier1-tier3 tests are still in progress. > > [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.01/ > [2] https://bugs.openjdk.java.net/browse/JDK-8240711 > > > Thank you, > Daniil > > >
RFR: 8240711: TestJstatdPort.java failed due to "ExportException: Port already in use:"
Please review the change [1] that fixes the intermittent failure of the test. The problem here is that if the RMI port is in use than the test keep waiting for "jstatd started (bound to " to appear in the process output and in this case It doesn't happen. at java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232) at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205) at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:133) at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:254) at jdk.test.lib.thread.ProcessThread$ProcessRunnable.xrun(ProcessThread.java:153) at jdk.test.lib.thread.XRun.run(XRun.java:40) at java.lang.Thread.run(java.base@15-internal/Thread.java:832) at jdk.test.lib.thread.TestThread.run(TestThread.java:123) Testing: Mach5 tests for sun/tools/jstatd/ successfully passed. Tier1-tier3 tests are still in progress. [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8240711 Thank you, Daniil
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa, Serguei and Alex,
Please review a new version of the webrev that includes the changes Yasumasa
suggested.
> Shutdown hook is already registered in c'tor of HotSpotAgent.
>It works same as shutdownServer(), so I think shutdown hook at SALauncher
> is not needed.
The shutdown hook registered in the HotSpotAgent c'tor only works for
non-servers, so we still need a
the shutdown hook for remote server being added in SALauncher. I changed it to
use the lambda expression.
101 public HotSpotAgent() {
102 // for non-server add shutdown hook to clean-up debugger in case
103 // of forced exit. For remote server, shutdown hook is added by
104 // DebugServer.
105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
106 new Runnable() {
107 public void run() {
108 synchronized (HotSpotAgent.this) {
109 if (!isServer) {
110 detach();
111 }
112 }
113 }
114 }));
115 }
>>Hmm... I think port check (already in use) is not needed because
>> test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
>> `exclusiveAccess.dirs=.` to avoid concurrent execution
As I understand exclusiveAccess.dirs prevents only the tests located in this
directory from being run simultaneously and other tests could still run in
parallel with one of these tests. Thus I would prefer to have the retry
mechanism in place. I simplified the code using the class variables instead of
local arrays.
Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd
tests) succeeded.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
[2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
[3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751
Thank you,
Daniil
On 3/6/20, 6:15 PM, "Yasumasa Suenaga" wrote:
Hi Daniil,
On 2020/03/07 3:38, Daniil Titov wrote:
> Hi Yasumasa,
>
> -> checkBasicOptions() is needed? I think you can remove this method
and embed it in caller.
> I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect (
it throws an exception if parameters are incorrect) and ignores its return
might look confusing. Thus, I found it more appropriate to wrap it inside a
method with more relevant name .
Ok, but I prefer to leave comment it.
> > SADebugDTest
> > - Why do you declare portInUse and testResult as array? Their length
is 1, so I think you don't need to use array.
> We cannot use primitives there since these local variables are captured
in lambda expression and are required to be final.
> The other option is to use some other wrapper for them but I don't see
any obvious benefits in it comparing to the array.
Hmm... I think port check (already in use) is not needed because
test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
`exclusiveAccess.dirs=.` to avoid concurrent execution.
If you do not think this error check, test code is more simply.
> I will include your other suggestion in the new version of the webrev.
Sorry, I have one more comment:
> - Shutdown hook is very good idea. You can implement more
simply if you use lambda expression.
Shutdown hook is already registered in c'tor of HotSpotAgent.
It works same as shutdownServer(), so I think shutdown hook at SALauncher
is not needed.
Thanks,
Yasumasa
> Thanks!
> Daniil
>
> On 3/6/20, 12:30 AM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
>
> - SALauncher.java
> - checkBasicOptions() is needed? I think you can remove this
method and embed it in caller.
> - I think registryPort should be checked with
Integer.parseInt() like others (rmiPort and pid) rather than regex.
> - Shutdown hook is very good idea. You can implement more
simply if you use lambda expression.
>
> - SADebugDTest.java
> - Please add bug ID to @bug.
> - Why do you declare portInUse and testResult as array? Their
length is 1, so I think you don't need to use array.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/03/06 10:15, Daniil Titov wrote:
> > Hi Yasumasa, Serguei and Alex,
> >
> > Please review a new version of the fix [1] that addresses your
comments. The new version in addition to RMI connector
> > port option intro
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa,
-> checkBasicOptions() is needed? I think you can remove this method and embed
it in caller.
I think that having a piece of code that invokes a method named
"buildAttachArgs" with a copy of the argument map just for its side-effect (
it throws an exception if parameters are incorrect) and ignores its return
might look confusing. Thus, I found it more appropriate to wrap it inside a
method with more relevant name .
> SADebugDTest
> - Why do you declare portInUse and testResult as array? Their length is 1,
> so I think you don't need to use array.
We cannot use primitives there since these local variables are captured in
lambda expression and are required to be final.
The other option is to use some other wrapper for them but I don't see any
obvious benefits in it comparing to the array.
I will include your other suggestion in the new version of the webrev.
Thanks!
Daniil
On 3/6/20, 12:30 AM, "Yasumasa Suenaga" wrote:
Hi Daniil,
- SALauncher.java
- checkBasicOptions() is needed? I think you can remove this method
and embed it in caller.
- I think registryPort should be checked with Integer.parseInt() like
others (rmiPort and pid) rather than regex.
- Shutdown hook is very good idea. You can implement more simply if
you use lambda expression.
- SADebugDTest.java
- Please add bug ID to @bug.
- Why do you declare portInUse and testResult as array? Their length
is 1, so I think you don't need to use array.
Thanks,
Yasumasa
On 2020/03/06 10:15, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
>
> Please review a new version of the fix [1] that addresses your comments.
The new version in addition to RMI connector
> port option introduces two more options to specify RMI registry port and
RMI connector host name. Currently, these
> last two settings could be specified using the system properties but the
system properties have the following disadvantages
> comparing to the command line options:
> - It’s hard to know about them: they are not listed in tool’s help.
> - They have long names that hard to remember
> - It is easy to mistype them in the command line and you will not
get any warning about it.
>
> The CSR [2] was also updated and needs to be reviewed.
>
> Testing: Manual testing with attaching the debug server to the running
Java process or to the core file inside a docker
> container and connecting to it with the GUI debugger. Mach5
tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.02/
> [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
> [3] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751
>
> Thank you,
> Daniil
>
> On 2/24/20, 5:45 AM, "Yasumasa Suenaga" wrote:
>
> Hi Daniil,
>
> - SALauncher::buildAttachArgs is not only to build arguments but
also to check consistency of arguments.
> Thus you should use buildAttachArgs() in runDEBUGD(). If you do
so, runDEBUGD() would be more simply.
>
> - SADebugDTest::testWithPidAndRmiPort would retry until
`--rmiport` can be used.
> But you can use same port number as RMI registry (1099).
> It is same as relation between jmxremote.port and
jmxremote.rmi.port.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2020/02/24 13:21, Daniil Titov wrote:
> > Please review change that adds a new command line option to jhsdb
tool for the debugd mode to specify a RMI connector port.
> > Currently a random port is used that prevents the debug server
from being used behind a firewall or in a container.
> >
> > New CSR [3] was created for this change and it needs to be
reviewed as well.
> >
> > Man pages for jhsdb will be updated in a separate issue.
> >
> > The current implementation (sun.jvm.hotspot.SALauncher) parses
the command line options passed to jhsdb tool,
> > converts them to the ones for the debug server and then delegates
the call to sun.jvm.hotspot.DebugServer.main().
> >
> >// delegate to the actual SA debug server.
> > 367 DebugServer.main(newArgArray.toArray(new String[0]));
> >
> > However, sun.jvm.hotspot.DebugServer doesn't support named
options and that prevents from efficiently adding new options to the tool.
Re: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Yasumasa, Serguei and Alex, Please review a new version of the fix [1] that addresses your comments. The new version in addition to RMI connector port option introduces two more options to specify RMI registry port and RMI connector host name. Currently, these last two settings could be specified using the system properties but the system properties have the following disadvantages comparing to the command line options: - It’s hard to know about them: they are not listed in tool’s help. - They have long names that hard to remember - It is easy to mistype them in the command line and you will not get any warning about it. The CSR [2] was also updated and needs to be reviewed. Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker container and connecting to it with the GUI debugger. Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.02/ [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 [3] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 Thank you, Daniil On 2/24/20, 5:45 AM, "Yasumasa Suenaga" wrote: Hi Daniil, - SALauncher::buildAttachArgs is not only to build arguments but also to check consistency of arguments. Thus you should use buildAttachArgs() in runDEBUGD(). If you do so, runDEBUGD() would be more simply. - SADebugDTest::testWithPidAndRmiPort would retry until `--rmiport` can be used. But you can use same port number as RMI registry (1099). It is same as relation between jmxremote.port and jmxremote.rmi.port. Thanks, Yasumasa On 2020/02/24 13:21, Daniil Titov wrote: > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > Currently a random port is used that prevents the debug server from being used behind a firewall or in a container. > > New CSR [3] was created for this change and it needs to be reviewed as well. > > Man pages for jhsdb will be updated in a separate issue. > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > >// delegate to the actual SA debug server. > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated > but I would prefer to address it in a separate issue. > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > container and connecting to it with the GUI debugger. > Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > Thank you, > Daniil > >
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Serguei, I will update the CSR and the fix to include this change. Thank you, Daniil On 2/25/20, 11:07 AM, "serguei.spit...@oracle.com" wrote: Hi Daniil, Thank you for reply. I agree with the approach to avoid using system properties. Then it is better to be consistent. I'd consider adding an RMI registry port option as well. Will look at your comments in the CSR and reply there. Thanks, Serguei On 2/25/20 10:05 AM, Daniil Titov wrote: > Hi Serguei, > > I added my comments there. In brief, I believe that in long term in the serviceability tools we should avoid > using the system properties and prefer the command line options instead. > > Thanks, > Daniil > > On 2/24/20, 11:04 AM, "serguei.spit...@oracle.com" wrote: > > Hi Daniil, > > I've looked at CSR and posted a couple of questions there. > It'd be nice if you help to resolve my confusion. :) > > Thanks, > Serguei > > > On 2/23/20 20:21, Daniil Titov wrote: > > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > > Currently a random port is used that prevents the debug server from being used behind a firewall or in a container. > > > > New CSR [3] was created for this change and it needs to be reviewed as well. > > > > Man pages for jhsdb will be updated in a separate issue. > > > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > > > >// delegate to the actual SA debug server. > > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > > and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated > > but I would prefer to address it in a separate issue. > > > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > > container and connecting to it with the GUI debugger. > > Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 > > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > > > Thank you, > > Daniil > > > > > > > >
Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Hi Serguei, I added my comments there. In brief, I believe that in long term in the serviceability tools we should avoid using the system properties and prefer the command line options instead. Thanks, Daniil On 2/24/20, 11:04 AM, "serguei.spit...@oracle.com" wrote: Hi Daniil, I've looked at CSR and posted a couple of questions there. It'd be nice if you help to resolve my confusion. :) Thanks, Serguei On 2/23/20 20:21, Daniil Titov wrote: > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. > Currently a random port is used that prevents the debug server from being used behind a firewall or in a container. > > New CSR [3] was created for this change and it needs to be reviewed as well. > > Man pages for jhsdb will be updated in a separate issue. > > The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, > converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). > >// delegate to the actual SA debug server. > 367 DebugServer.main(newArgArray.toArray(new String[0])); > > However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. > I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher > and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated > but I would prefer to address it in a separate issue. > > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker > container and connecting to it with the GUI debugger. > Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 > > Thank you, > Daniil > >
RFR: 8196751: Add jhsdb option to specify debug server RMI connector port
Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port. Currently a random port is used that prevents the debug server from being used behind a firewall or in a container. New CSR [3] was created for this change and it needs to be reviewed as well. Man pages for jhsdb will be updated in a separate issue. The current implementation (sun.jvm.hotspot.SALauncher) parses the command line options passed to jhsdb tool, converts them to the ones for the debug server and then delegates the call to sun.jvm.hotspot.DebugServer.main(). // delegate to the actual SA debug server. 367 DebugServer.main(newArgArray.toArray(new String[0])); However, sun.jvm.hotspot.DebugServer doesn't support named options and that prevents from efficiently adding new options to the tool. I found it more suitable to start Hotspot agent directly in SALauncher rather than adding a new option in both sun.jvm.hotspot.SALauncher and sun.jvm.hotspot.DebugServer and delegating the call. With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated but I would prefer to address it in a separate issue. Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker container and connecting to it with the GUI debugger. Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751 [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831 Thank you, Daniil
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Thank you Chris and Serguei for reviewing this change!
Best regards,
Daniil
On 2/5/20, 11:15 AM, "Chris Plummer" wrote:
+1
Chris
On 2/5/20 9:36 AM, serguei.spit...@oracle.com wrote:
> Hi Daniil,
>
> Looks good.
> Thank you for the update!
>
> Thanks,
> Serguei
>
>
> On 2/4/20 22:00, Daniil Titov wrote:
>> Hi Serguei,
>>
>> Thank you for finding this! Please review the new version of webrev [1]
>> that has it corrected. The new webrev also includes changes in the
>> test to
>> make sure that all jstatd tests run for both styles of command line
>> options.
>> Testing: Mach5 jobs for sun/tools/jstatd succeeded. Tiers1,
>> tiers2, tiers3,
>> and tiers5 job are in the progress.
>>
>> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196729/webrev.02/
>> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196729
>> [3] CSR : https://bugs.openjdk.java.net/browse/JDK-8238357
>>
>> Thanks,
>> Daniil
>>
>> From: "serguei.spit...@oracle.com"
>> Date: Tuesday, February 4, 2020 at 7:51 PM
>> To: Daniil Titov ,
>> "serviceability-dev@openjdk.java.net"
>>
>> Subject: Re: RFR: 8196729: Add jstatd option to specify RMI connector
>> port
>>
>> Hi Daniil,
>>
>> It looks okay to me in general.
>> But I'm puzzled with this part:
>>
>>
http://cr.openjdk.java.net/~dtitov/8196729/webrev.01/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java.udiff.html
>>
>> +} else if (arg.startsWith("-r")) {
>> +if (arg.compareTo("-r") != 0) {
>> +port = Integer.parseInt(arg.substring(2));
>> +} else {
>> +argc++;
>> +if (argc >= args.length) {
>> +printUsage();
>> +System.exit(1);
>> +}
>> +rmiPort = Integer.parseInt(args[argc]);
>> +}
>>
>> The option -r is for rmi connection port number.
>> Why does this code set the RMI registry port? :
>> +if (arg.compareTo("-r") != 0) {
>> +port = Integer.parseInt(arg.substring(2));
>>
>> Thanks,
>> Serguei
>>
>>
>> On 1/31/20 13:08, Daniil Titov wrote:
>> Please review change [1] that adds a new command line option to
>> jstatd tool to specify a RMI connector port.
>>
>> Currently a random port is used that prevents this tool from being
>> used behind a firewall or in a container.
>>
>> New CSR [3] was created for this change and it needs to be reviewed
>> as well.
>>
>> Man pages for jstatd will be updated in a separate issue.
>>
>> Testing: Mach5 tier1-tier3 and sun/tools/jstatd/* tests succeeded.
>> Mach5 tier5 tests are in progress.
>> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196729/webrev.01/
>> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196729
>> [3] CSR : https://bugs.openjdk.java.net/browse/JDK-8238357
>>
>> Thank you,
>> Daniil
>>
>>
>>
>>
>>
>>
>>
>
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Hi Serguei,
Thank you for finding this! Please review the new version of webrev [1]
that has it corrected. The new webrev also includes changes in the test to
make sure that all jstatd tests run for both styles of command line options.
Testing: Mach5 jobs for sun/tools/jstatd succeeded. Tiers1, tiers2, tiers3,
and tiers5 job are in the progress.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8196729/webrev.02/
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196729
[3] CSR : https://bugs.openjdk.java.net/browse/JDK-8238357
Thanks,
Daniil
From: "serguei.spit...@oracle.com"
Date: Tuesday, February 4, 2020 at 7:51 PM
To: Daniil Titov ,
"serviceability-dev@openjdk.java.net"
Subject: Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Hi Daniil,
It looks okay to me in general.
But I'm puzzled with this part:
http://cr.openjdk.java.net/~dtitov/8196729/webrev.01/src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java.udiff.html
+} else if (arg.startsWith("-r")) {
+if (arg.compareTo("-r") != 0) {
+port = Integer.parseInt(arg.substring(2));
+} else {
+argc++;
+if (argc >= args.length) {
+printUsage();
+System.exit(1);
+}
+rmiPort = Integer.parseInt(args[argc]);
+}
The option -r is for rmi connection port number.
Why does this code set the RMI registry port? :
+if (arg.compareTo("-r") != 0) {
+port = Integer.parseInt(arg.substring(2));
Thanks,
Serguei
On 1/31/20 13:08, Daniil Titov wrote:
Please review change [1] that adds a new command line option to jstatd tool to
specify a RMI connector port.
Currently a random port is used that prevents this tool from being used behind
a firewall or in a container.
New CSR [3] was created for this change and it needs to be reviewed as well.
Man pages for jstatd will be updated in a separate issue.
Testing: Mach5 tier1-tier3 and sun/tools/jstatd/* tests succeeded. Mach5 tier5
tests are in progress.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8196729/webrev.01/
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196729
[3] CSR : https://bugs.openjdk.java.net/browse/JDK-8238357
Thank you,
Daniil
Re: RFR: 8196729: Add jstatd option to specify RMI connector port
Hi Chris, Thank you for describing this in such details! Your description is correct. In addition, apart from jstat there is jps tool that also can communicate with jstatd and currently it faces the same problems if jstatd is deployed behind a firewall or in a container: after successful connection to RMI registry the further communication fails since jstatd chooses a random RMI port that is not published by the container or might be blocked by a firewall configuration. Best regards, Daniil On 1/31/20, 1:45 PM, "Chris Plummer" wrote: Hi Daniil, Just want to make sure I understand what communications are going on here. Your concern is when the jstat and jstatd processes are on different sides of the firewall. When you launch jstatd, you specify the socket port it will receive requests on, and when you launch jstat, you must specify this same socket port, so no firewall problem there assuming the firewall is configured to allow communication over that port. However, once the request is received by jstatd, data can be communicated via RMI rather than over the specified socket port. By default jstatd was choosing a random RMI port, and I assume this RMI port was communicated to the jstat process via the initial socket port. This presents a problem for firewall configuration, since the firewall configuration cannot know the RMI port that will be used. So now you're allowing the rmi port to also be specified. Am I close? :) Chris On 1/31/20 1:08 PM, Daniil Titov wrote: > Please review change [1] that adds a new command line option to jstatd tool to specify a RMI connector port. > > Currently a random port is used that prevents this tool from being used behind a firewall or in a container. > > New CSR [3] was created for this change and it needs to be reviewed as well. > > Man pages for jstatd will be updated in a separate issue. > > Testing: Mach5 tier1-tier3 and sun/tools/jstatd/* tests succeeded. Mach5 tier5 tests are in progress. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196729/webrev.01/ > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196729 > [3] CSR : https://bugs.openjdk.java.net/browse/JDK-8238357 > > Thank you, > Daniil > > >
RFR: 8196729: Add jstatd option to specify RMI connector port
Please review change [1] that adds a new command line option to jstatd tool to specify a RMI connector port. Currently a random port is used that prevents this tool from being used behind a firewall or in a container. New CSR [3] was created for this change and it needs to be reviewed as well. Man pages for jstatd will be updated in a separate issue. Testing: Mach5 tier1-tier3 and sun/tools/jstatd/* tests succeeded. Mach5 tier5 tests are in progress. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196729/webrev.01/ [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196729 [3] CSR : https://bugs.openjdk.java.net/browse/JDK-8238357 Thank you, Daniil
Re: RFR: 8235681: Remove unnecessary workarounds in UnixOperatingSystem.c
Thank you Alex and Chris for reviewing this change!
Best regards,
Daniil
On 1/24/20, 10:38 AM, "Alex Menkov" wrote:
+1
--alex
On 01/22/2020 19:02, Chris Plummer wrote:
> Looks good.
>
> Chris
>
> On 1/22/20 3:30 PM, Daniil Titov wrote:
>> Hi Chris,
>>
>> Please review a new version of the fix [1] that converts the check to
>> the assert.
>>
>>> Also, I'm not clear on the need for the *pkernelLoad
>>> initialization. It
>>> seems this is attempting to fix a different issue, but it's already
>>> initialized to 0 at the start of the function.
>> You are right, *pkernelLoad is already initialized on line 251. The
>> new version of
>> the webrev does not include this change.
>>
>> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8235681/webrev.02/
>> [2] Bug: https://bugs.openjdk.java.net/browse/JDK-8235681
>>
>> Thank you,
>> Daniil
>>
>> On 1/21/20, 10:38 AM, "Chris Plummer" wrote:
>>
>> Hi Daniil,
>> Do you think it's worth changing the check to an assert instead of
>> removing it?
>> Also, I'm not clear on the need for the *pkernelLoad
>> initialization. It
>> seems this is attempting to fix a different issue, but it's already
>> initialized to 0 at the start of the function.
>> thanks,
>> Chris
>> On 1/17/20 7:10 PM, Daniil Titov wrote:
>> > Please review a change that removes unnecessary workaround in
>> UnixOperatingSystem.c.
>> >
>> > It looks as this workaround
>> >
>> > if (pticks->usedKernel < tmp.usedKernel) {
>> > kdiff = 0;
>> > }
>> >
>> > was added to compensate a missed initialization of
>> counters.jvmTicks that resulted in the new
>> > counters being compared with the garbage when
>> get_cpuload_internal(...) was called for the first time.
>> >
>> > This missed initialization was fixed in [3].
>> >
>> > Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker
>> tests successfully passed.
>> >
>> > [1] Webrev : http://cr.openjdk.java.net/~dtitov/8235681/webrev.01/
>> > [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8235681
>> > [3] https://bugs.openjdk.java.net/browse/JDK-8226575
>> >
>> > Thank you,
>> > Daniil
>> >
>> >
>>
>>
>
>
Re: RFR: 8235681: Remove unnecessary workarounds in UnixOperatingSystem.c
Hi Chris,
Please review a new version of the fix [1] that converts the check to the
assert.
>Also, I'm not clear on the need for the *pkernelLoad initialization. It
>seems this is attempting to fix a different issue, but it's already
>initialized to 0 at the start of the function.
You are right, *pkernelLoad is already initialized on line 251. The new version
of
the webrev does not include this change.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8235681/webrev.02/
[2] Bug: https://bugs.openjdk.java.net/browse/JDK-8235681
Thank you,
Daniil
On 1/21/20, 10:38 AM, "Chris Plummer" wrote:
Hi Daniil,
Do you think it's worth changing the check to an assert instead of
removing it?
Also, I'm not clear on the need for the *pkernelLoad initialization. It
seems this is attempting to fix a different issue, but it's already
initialized to 0 at the start of the function.
thanks,
Chris
On 1/17/20 7:10 PM, Daniil Titov wrote:
> Please review a change that removes unnecessary workaround in
UnixOperatingSystem.c.
>
> It looks as this workaround
>
> if (pticks->usedKernel < tmp.usedKernel) {
> kdiff = 0;
> }
>
> was added to compensate a missed initialization of counters.jvmTicks that
resulted in the new
> counters being compared with the garbage when get_cpuload_internal(...)
was called for the first time.
>
> This missed initialization was fixed in [3].
>
> Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker tests
successfully passed.
>
> [1] Webrev : http://cr.openjdk.java.net/~dtitov/8235681/webrev.01/
> [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8235681
> [3] https://bugs.openjdk.java.net/browse/JDK-8226575
>
> Thank you,
> Daniil
>
>
RFR: 8235681: Remove unnecessary workarounds in UnixOperatingSystem.c
Please review a change that removes unnecessary workaround in
UnixOperatingSystem.c.
It looks as this workaround
if (pticks->usedKernel < tmp.usedKernel) {
kdiff = 0;
}
was added to compensate a missed initialization of counters.jvmTicks that
resulted in the new
counters being compared with the garbage when get_cpuload_internal(...) was
called for the first time.
This missed initialization was fixed in [3].
Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker tests
successfully passed.
[1] Webrev : http://cr.openjdk.java.net/~dtitov/8235681/webrev.01/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8235681
[3] https://bugs.openjdk.java.net/browse/JDK-8226575
Thank you,
Daniil
Re: 8236873: Worker has a deadlock bug
Thank you David, Daniel, and Serguei for reviewing this change!
Best regards,
Daniil
On 1/15/20, 10:45 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
LGTM++
Thanks,
Serguei
On 1/15/20 14:28, David Holmes wrote:
> +1
>
> David
>
> On 16/01/2020 4:41 am, Daniel Fuchs wrote:
>> Hi Daniil,
>>
>> That looks fine to me.
>>
>> best regards,
>>
>> -- daniel
>>
>> On 15/01/2020 18:15, Daniil Titov wrote:
>>> Please review a change [1] that fixes a deadlock issue [2] in
>>> sun.tools.jconsole.Worker class.
>>>
>>> There is no need in guarding "stopped" flag by a lock. The fix
>>> removes this excessive locking and
>>> instead makes the flag volatile.
>>>
>>> Mach5 tier1-tier3 tests passed.
>>>
>>> [1] Webrev : http://cr.openjdk.java.net/~dtitov/8236873/webrev.01/
>>> [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8236873
>>>
>>> Best regards,
>>> Daniil
>>>
>>>
>>
RFR: 8236873: Worker has a deadlock bug
Please review a change [1] that fixes a deadlock issue [2] in sun.tools.jconsole.Worker class. There is no need in guarding "stopped" flag by a lock. The fix removes this excessive locking and instead makes the flag volatile. Mach5 tier1-tier3 tests passed. [1] Webrev : http://cr.openjdk.java.net/~dtitov/8236873/webrev.01/ [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8236873 Best regards, Daniil
Re: RFR: 8213222: remove RMIConnectorServer.CREDENTIAL_TYPES
Hi Alan and Daniel, Thank you for reviewing this change and the CSR. I updated the release note as Daniel suggested. Best regards, Daniil On 1/13/20, 7:36 AM, "Daniel Fuchs" wrote: Hi Daniil, Looks good to me as well. I wonder however if the release note should point to the replacement? Something like: The terminally deprecated constant `javax.management.remote.rmi.RMIConnectorServer.CREDENTIAL_TYPE` has been removed. A filter pattern can be specified instead using `RMIConnectorServer.CREDENTIALS_FILTER_PATTERN`. best regards, -- daniel On 11/01/2020 04:52, Daniil Titov wrote: > Please review a change [1] that removes constant javax.management.remote.rmi.RMIConnectorServer.CREDENTIAL_TYPE. > This constant represents the name of the attribute that specifies a list of class names acceptable to the RMIServer.newClient() > remote method call. This constant was superseded by RMIConnectorServer.CREDENTIALS_FILTER_PATTERN and marked as > deprecated for removal in JDK 10 [3]. > > A new CRS [4] and a release note sub-task [5] were also created. Please review the CSR [4] as well. > > Text search over the repository didn't find any other usage of this constant. Tier1 - tier3 Mach5 tests succeeded. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8213222/webrev.01/ > [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8213222 > [3] https://bugs.openjdk.java.net/browse/JDK-8191313 > [4] CSR: https://bugs.openjdk.java.net/browse/JDK-8236954 > [5] Release Note sub-task: https://bugs.openjdk.java.net/browse/JDK-8236955 > > Thank you, > Daniil > >
RFR: 8213222: remove RMIConnectorServer.CREDENTIAL_TYPES
Please review a change [1] that removes constant javax.management.remote.rmi.RMIConnectorServer.CREDENTIAL_TYPE. This constant represents the name of the attribute that specifies a list of class names acceptable to the RMIServer.newClient() remote method call. This constant was superseded by RMIConnectorServer.CREDENTIALS_FILTER_PATTERN and marked as deprecated for removal in JDK 10 [3]. A new CRS [4] and a release note sub-task [5] were also created. Please review the CSR [4] as well. Text search over the repository didn't find any other usage of this constant. Tier1 - tier3 Mach5 tests succeeded. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8213222/webrev.01/ [2] Issue: https://bugs.openjdk.java.net/browse/JDK-8213222 [3] https://bugs.openjdk.java.net/browse/JDK-8191313 [4] CSR: https://bugs.openjdk.java.net/browse/JDK-8236954 [5] Release Note sub-task: https://bugs.openjdk.java.net/browse/JDK-8236955 Thank you, Daniil
Re: 8236190 : Unproblem list vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t002/TestDescription.java
Hi Chris, Igor, and Alex, Thank you for reviewing this change! Best regards, Daniil On 12/20/19, 5:18 PM, "Alex Menkov" wrote: +1 --alex On 12/20/2019 17:12, Chris Plummer wrote: > Looks good. > > Chris > > On 12/20/19 4:42 PM, Daniil Titov wrote: >> Please a review a changeset below that removes >> vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t002/TestDescription.java >> >> and runtime/appcds/cacheObject/RedefineClassTest.java tests from >> test/hotspot/jtreg/ProblemList-graal.txt. >> >> >> --- a/test/hotspot/jtreg/ProblemList-graal.txt Wed Dec 11 19:20:57 >> 2019 -0800 >> +++ b/test/hotspot/jtreg/ProblemList-graal.txt Fri Dec 20 11:53:31 >> 2019 -0800 >> @@ -147,9 +147,7 @@ >> >> vmTestbase/nsk/jvmti/ForceEarlyReturn/ForceEarlyReturn001/TestDescription.java >> 8195674,8195635 generic-all >> >> vmTestbase/nsk/jvmti/ForceEarlyReturn/ForceEarlyReturn002/TestDescription.java >> 8195674,8195635 generic-all >> -runtime/appcds/cacheObject/RedefineClassTest.java >> 8204506 macosx-all >> -vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java >> 8204506,8195635 macosx-all,generic-all >> -vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t002/TestDescription.java >> 8204506 macosx-all >> +vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java >> 8195635 generic-all >> >> >> These tests were added in ProblemList-graal.txt in [1] and [2] but >> issue [1] is no longer reproducible in JDK 15 and the tests run fine >> in Mach5. >> >> The third test >> vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java >> appears to run fine as well but it was problem-listed >> due to several issues and issue [3] is not resolved yet. Thus I >> decided to keep this test on the problem list. >> >> Mach5 tests for tier1,tier2, and tier3 successfully passed. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8204506 >> [2] https://bugs.openjdk.java.net/browse/JDK-8209587 >> [3] https://bugs.openjdk.java.net/browse/JDK-8195635 >> [4] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8236190 >> >> Thanks. >> Daniil >> >> >
RFR: 8236190 : Unproblem list vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t002/TestDescription.java
Please a review a changeset below that removes vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t002/TestDescription.java and runtime/appcds/cacheObject/RedefineClassTest.java tests from test/hotspot/jtreg/ProblemList-graal.txt. --- a/test/hotspot/jtreg/ProblemList-graal.txt Wed Dec 11 19:20:57 2019 -0800 +++ b/test/hotspot/jtreg/ProblemList-graal.txt Fri Dec 20 11:53:31 2019 -0800 @@ -147,9 +147,7 @@ vmTestbase/nsk/jvmti/ForceEarlyReturn/ForceEarlyReturn001/TestDescription.java 8195674,8195635 generic-all vmTestbase/nsk/jvmti/ForceEarlyReturn/ForceEarlyReturn002/TestDescription.java 8195674,8195635 generic-all -runtime/appcds/cacheObject/RedefineClassTest.java 8204506 macosx-all -vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java 8204506,8195635 macosx-all,generic-all -vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t002/TestDescription.java 8204506 macosx-all +vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java 8195635 generic-all These tests were added in ProblemList-graal.txt in [1] and [2] but issue [1] is no longer reproducible in JDK 15 and the tests run fine in Mach5. The third test vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java appears to run fine as well but it was problem-listed due to several issues and issue [3] is not resolved yet. Thus I decided to keep this test on the problem list. Mach5 tests for tier1,tier2, and tier3 successfully passed. [1] https://bugs.openjdk.java.net/browse/JDK-8204506 [2] https://bugs.openjdk.java.net/browse/JDK-8209587 [3] https://bugs.openjdk.java.net/browse/JDK-8195635 [4] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8236190 Thanks. Daniil
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
Hi Bob, David, Mandy, and Serguei,
Thank you for reviewing this change!
Best regards,
Daniil
On 12/11/19, 9:21 AM, "Bob Vandette" wrote:
Yes, I defer to Mandy on the best way to express the various Java
exceptions.
I’m ok with the changes.
Thanks for getting this done for JDK14!
Bob.
> On Dec 11, 2019, at 12:12 PM, Daniil Titov
wrote:
>
> Hi Serguei,
>
> Thank you for your comments. I will correct this nits before pushing the
changes.
>
> Hi Bob and David,
>
>> [Mandy Chung]
>>> I reviewed Metrics and Subsystem in this version.
>>> I don't need to see a new webrev.
>
> As I understood Mandy finished reviewing this fix. Just wanted to confirm
with you if you are okey with that version of the fix (webrev.06) ?
>
> Mach5 testing: tier1-tier6 and open/test/hotspot/jtreg/containers/docker
tests passed.
>
> Thank you,
> Daniil
>
>
>
> On 12/9/19, 6:02 PM, "serguei.spit...@oracle.com"
wrote:
>
>Hi Daniil,
>
>It is not a full review, just some minor comments.
>In fact, I do not see real problems yet.
>
>
http://cr.openjdk.java.net/~dtitov/8226575/webrev.05/src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java.frames.html
>
> 55 public long getTotalSwapSpaceSize() {
> 56 if (containerMetrics != null) {
> 57 long limit =
containerMetrics.getMemoryAndSwapLimit();
> 58 // The memory limit metrics is not available if JVM
>runs on Linux host ( not in a docker container)
> 59 // or if a docker container was started without
>specifying a memory limit ( without '--memory='
> 60 // Docker option). In latter case there is no limit
on
>how much memory the container can use and
> 61 // it can use as much memory as the host's OS allows.
> 62 long memLimit = containerMetrics.getMemoryLimit();
> 63 if (limit >= 0 && memLimit >= 0) {
> 64 return limit - memLimit;
> 65 }
> 66 }
> 67 return getTotalSwapSpaceSize0();
> 68 }
>
> Unneeded space after brackets '('.
> Do we need to check if the (limit - memLimit) value is negative?
> The same question is for getFreeSwapSpaceSize():
> memSwapLimit - memLimit - (memSwapUsage - memUsage)
>
> and getFreeMemorySize():
> 101 return limit - usage;
>
> 81 // If this happens just retry the loop
for
>a few iterations
>
> Dot is missed at the end of comment.
>
>
>
http://cr.openjdk.java.net/~dtitov/8226575/webrev.05/test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java.html
>
> 34 System.out.println(String.format("Runtime.availableProcessors:
>%d", Runtime.getRuntime().availableProcessors()));
> 35
>
System.out.println(String.format("OperatingSystemMXBean.getAvailableProcessors:
>%d", osBean.getAvailableProcessors()));
> 36
>
System.out.println(String.format("OperatingSystemMXBean.getTotalMemorySize:
>%d", osBean.getTotalMemorySize()));
> 37
>
System.out.println(String.format("OperatingSystemMXBean.getTotalPhysicalMemorySize:
>%d", osBean.getTotalPhysicalMemorySize()));
> 38
>
System.out.println(String.format("OperatingSystemMXBean.getFreeMemorySize:
>%d", osBean.getFreeMemorySize()));
> 39
>
System.out.println(String.format("OperatingSystemMXBean.getFreePhysicalMemorySize:
>%d", osBean.getFreePhysicalMemorySize()));
> 40
>
System.out.println(String.format("OperatingSystemMXBean.getTotalSwapSpaceSize:
>%d", osBean.getTotalSwapSpaceSize()));
> 41
>
System.out.println(String.format("OperatingSystemMXBean.getFreeSwapSpaceSize:
>%d", osBean.getFreeSwapSpaceSize()));
> 42
>System.out.println(String.format("OperatingSystemMXBean.getCpuLoad:
%f",
>osBean.getCpuLoad()));
> 43
>
System.out.println(String.format("OperatingSystemMXBean.getSystemCpuLoad:
>%f", osBean.getSystemCpuLoad()));
>
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
Hi Serguei,
Thank you for reviewing this change.
Just wanted to add that the only "volatile" metrics are "usage" ones (
memoryUsage and
memoryAndSwapLimit). The "limit" metrics (memoryLimit and memoryAndSwapLimit)
are set
when the container starts and are not subjects to change. The only method that
reads more than one
"volatile" metric is getFreeSwapSpaceSize() and it has a code that retries if
the calculated swapUsage
is negative as a result of non-atomic reads.
Thank you,
Daniil
On 12/11/19, 3:13 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
One my concerns was a non-atomic read of multiple metrics before comparison.
It creates a potential to get a mismatch in result.
However, the probability to get a negative value is pretty low, I think.
The other concern (if incorrect metrics are returned) is covered by
JDK-8235522.
Revising all concerns in JDK-8235522 sounds good to me.
Thanks,
Serguei
On 12/10/19 10:29, Daniil Titov wrote:
> Hi Serguei,
>
>>Do we need to check if the (limit - memLimit) value is negative?
>>The same question is for getFreeSwapSpaceSize():
>> memSwapLimit - memLimit - (memSwapUsage - memUsage)
>>
>>and getFreeMemorySize():
>> 101 return limit - usage;
> I don't think we need such check here. If it happens in fact it means the
serious system malfunction and a negative value this method
> returns would indicate this (currently the native methods already returns
-1 if something went wrong). But we could revise it in the follow
> up issue I created for that [1].
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8235522
>
> Thank you,
> Daniil
>
> On 12/9/19, 6:02 PM, "serguei.spit...@oracle.com"
wrote:
>
> Hi Daniil,
>
> It is not a full review, just some minor comments.
> In fact, I do not see real problems yet.
>
>
http://cr.openjdk.java.net/~dtitov/8226575/webrev.05/src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java.frames.html
>
> 55 public long getTotalSwapSpaceSize() {
> 56 if (containerMetrics != null) {
> 57 long limit =
containerMetrics.getMemoryAndSwapLimit();
> 58 // The memory limit metrics is not available if JVM
> runs on Linux host ( not in a docker container)
> 59 // or if a docker container was started without
> specifying a memory limit ( without '--memory='
> 60 // Docker option). In latter case there is no
limit on
> how much memory the container can use and
> 61 // it can use as much memory as the host's OS
allows.
> 62 long memLimit = containerMetrics.getMemoryLimit();
> 63 if (limit >= 0 && memLimit >= 0) {
> 64 return limit - memLimit;
> 65 }
> 66 }
> 67 return getTotalSwapSpaceSize0();
> 68 }
>
> Unneeded space after brackets '('.
> Do we need to check if the (limit - memLimit) value is negative?
> The same question is for getFreeSwapSpaceSize():
> memSwapLimit - memLimit - (memSwapUsage - memUsage)
>
> and getFreeMemorySize():
> 101 return limit - usage;
>
> 81 // If this happens just retry the loop
for
> a few iterations
>
> Dot is missed at the end of comment.
>
>
>
http://cr.openjdk.java.net/~dtitov/8226575/webrev.05/test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java.html
>
> 34 System.out.println(String.format("Runtime.availableProcessors:
> %d", Runtime.getRuntime().availableProcessors()));
> 35
>
System.out.println(String.format("OperatingSystemMXBean.getAvailableProcessors:
> %d", osBean.getAvailableProcessors()));
> 36
>
System.out.println(String.format("OperatingSystemMXBean.getTotalMemorySize:
> %d", osBean.getTotalMemorySize()));
> 37
>
System.out.println(String.format("OperatingSystemMXBean.getTotalPhysicalMemorySize:
> %d", osBean.getTotalPhysicalMemorySize()));
> 38
>
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
Typo fixed...
.. that the only "volatile" metrics are "usage" ones ( memoryUsage and
*memoryAndSwapUsage*).
Best regards,
Daniil
On 12/11/19, 3:33 PM, "Daniil Titov" wrote:
Hi Serguei,
Thank you for reviewing this change.
Just wanted to add that the only "volatile" metrics are "usage" ones (
memoryUsage and
memoryAndSwapLimit). The "limit" metrics (memoryLimit and
memoryAndSwapLimit) are set
when the container starts and are not subjects to change. The only method
that reads more than one
"volatile" metric is getFreeSwapSpaceSize() and it has a code that retries
if the calculated swapUsage
is negative as a result of non-atomic reads.
Thank you,
Daniil
On 12/11/19, 3:13 PM, "serguei.spit...@oracle.com"
wrote:
Hi Daniil,
One my concerns was a non-atomic read of multiple metrics before
comparison.
It creates a potential to get a mismatch in result.
However, the probability to get a negative value is pretty low, I think.
The other concern (if incorrect metrics are returned) is covered by
JDK-8235522.
Revising all concerns in JDK-8235522 sounds good to me.
Thanks,
Serguei
On 12/10/19 10:29, Daniil Titov wrote:
> Hi Serguei,
>
>>Do we need to check if the (limit - memLimit) value is
negative?
>>The same question is for getFreeSwapSpaceSize():
>> memSwapLimit - memLimit - (memSwapUsage - memUsage)
>>
>>and getFreeMemorySize():
>> 101 return limit - usage;
> I don't think we need such check here. If it happens in fact it means
the serious system malfunction and a negative value this method
> returns would indicate this (currently the native methods already
returns -1 if something went wrong). But we could revise it in the follow
> up issue I created for that [1].
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8235522
>
> Thank you,
> Daniil
>
> On 12/9/19, 6:02 PM, "serguei.spit...@oracle.com"
wrote:
>
> Hi Daniil,
>
> It is not a full review, just some minor comments.
> In fact, I do not see real problems yet.
>
>
http://cr.openjdk.java.net/~dtitov/8226575/webrev.05/src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java.frames.html
>
> 55 public long getTotalSwapSpaceSize() {
> 56 if (containerMetrics != null) {
> 57 long limit =
containerMetrics.getMemoryAndSwapLimit();
> 58 // The memory limit metrics is not available
if JVM
> runs on Linux host ( not in a docker container)
> 59 // or if a docker container was started without
> specifying a memory limit ( without '--memory='
> 60 // Docker option). In latter case there is no
limit on
> how much memory the container can use and
> 61 // it can use as much memory as the host's OS
allows.
> 62 long memLimit =
containerMetrics.getMemoryLimit();
> 63 if (limit >= 0 && memLimit >= 0) {
> 64 return limit - memLimit;
> 65 }
> 66 }
> 67 return getTotalSwapSpaceSize0();
> 68 }
>
> Unneeded space after brackets '('.
> Do we need to check if the (limit - memLimit) value is
negative?
> The same question is for getFreeSwapSpaceSize():
> memSwapLimit - memLimit - (memSwapUsage - memUsage)
>
> and getFreeMemorySize():
> 101 return limit - usage;
>
> 81 // If this happens just retry the
loop for
> a few iterations
>
> Dot is missed at the end of comment.
>
>
>
http://cr.openjdk.java.net/~dtitov/8226575/webrev.05/test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java.html
>
> 34
System.out.println(String.format("Runtime.availableProcessors:
> %d", Runtime.getRuntime().availableProcessors()));
>
Re: RFR(S): 8234277:ClhsdbLauncher should enable verbose exceptions and do a better job of detecting SA failures
Hi Chris, The change looks good to me. Best regards, Daniil On 12/4/19, 5:39 PM, "serviceability-dev on behalf of Chris Plummer" wrote: Can I get one more review please? thanks, Chris On 12/3/19 1:10 PM, serguei.spit...@oracle.com wrote: > Hi Chris, > > It looks good. > > Thanks, > Serguei > > On 12/3/19 12:45 PM, Chris Plummer wrote: >> Hello, >> >> Please review the following: >> >> https://bugs.openjdk.java.net/browse/JDK-8234277 >> http://cr.openjdk.java.net/~cjplummer/8234277/webrev.00/ >> >> No longer redirect stderr for the jhsdb/clhsdb process. It results in >> not seeing attach failures in the output, so OutputAnalyer can't >> check for them. >> >> Execute "verbose true" as the first clhsdb command after launching. >> This will result in verboseExceptions being true in >> CommandProcessor.java, so full exception traces will appear in the >> output. This will make debugging future SA test failures a lot easier. >> >> Add an extra check for any DebuggerException. This is mainly for >> detecting that the attached failed. This previously was going >> un-noticed, and instead the test would later fail because it noticed >> some other issue, like missing output, which isn't very informative. >> >> Add checks for other unexpected SA exceptions that are caught and >> printed by CommandProcessor. These will always have an "Error: " >> prefix, making them easy to detect. >> >> Problem list ClhsdbScanOops.java. With the new error checking, it >> will now always fail on windows due to JDK-8230731 and on macos and >> linux due to JDK-8235220. These failures are not "new" per se, but >> are just now being properly detected. >> >> thanks, >> >> Chris >
Re: 8226575: OperatingSystemMXBean should be made container aware
atingSystemMXBean.getFreeSwapSpaceSize:
%d", osBean.getFreeSwapSpaceSize()));
42 log(String.format("OperatingSystemMXBean.getCpuLoad: %f",
osBean.getCpuLoad()));
43 log(String.format("OperatingSystemMXBean.getSystemCpuLoad: %f",
osBean.getSystemCpuLoad()));
Thanks,
Serguei
On 12/6/19 17:41, Daniil Titov wrote:
> Hi David, Mandy, and Bob,
>
> Thank you for reviewing this fix.
>
> Please review a new version of the fix [1] that includes the following
changes comparing to the previous version of the webrev ( webrev.04)
> 1. The changes in Javadoc made in the webrev.04 comparing to webrev.03
and to CSR [3] were discarded.
> 2. The implementation of methods getFreeMemorySize, getTotalMemorySize,
getFreeSwapSpaceSize and getTotalSwapSpaceSize
> was also reverted to webrev.03 version that return host's values if
the metrics are unavailable or cannot be properly read.
> I would like to mention that currently the native implementation
of these methods de-facto may return -1 at some circumstances,
> but I agree that the changes proposed in the previous version of
the webrev increase such probability.
> I filed the follow-up issue [4] as Mandy suggested.
> 3. The legacy methods were renamed as David suggested.
>
>
>> src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
>> ! static int initialized=1;
>>
>> Am I reading this right that the code currently fails to actually do
the
>> initialization because of this ???
> Yes, currently the code fails to do the initialization but it was
unnoticed since method
> get_cpuload_internal(...) was never called for a specific CPU, the first
parameter "which"
> was always -1.
>
>> test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java
>>
>> System.out.println(String.format(...)
>>
>> Why not simply
>>
>> System.out.printf(..)
> As I tried explain it earlier it would make the tests unstable.
> System.out.printf(...) just delegates the call to System.out.format(...)
that doesn't emit the string atomically.
> Instead it parses the format string into a list of FormatString objects
and then iterates over the list.
> As a result, the other traces occasionally got printed between these
iterations and break the pattern the test is expected to find
> in the output.
>
> For example, here is the sample of a such output that has the trace
message printed between " OperatingSystemMXBean.getFreePhysicalMemorySize:"
> and "1030762496".
>
>
> [0.304s][trace][os,container] Memory Usage is: 42983424
> OperatingSystemMXBean.getFreeMemorySize: 1030758400
> [0.305s][trace][os,container] Path to /memory.usage_in_bytes is
/sys/fs/cgroup/memory/memory.usage_in_bytes
> [0.305s][trace][os,container] Memory Usage is: 42979328
> [0.306s][trace][os,container] Path to /memory.usage_in_bytes is
/sys/fs/cgroup/memory/memory.usage_in_bytes
> OperatingSystemMXBean.getFreePhysicalMemorySize:
[0.306s][trace][os,container] Memory Usage is: 42975232
> 1030762496
> OperatingSystemMXBean.getTotalSwapSpaceSize: 499122176
>
>
> java.lang.RuntimeException:
'OperatingSystemMXBean\\.getFreePhysicalMemorySize: [1-9][0-9]+' missing from
stdout/stderr
>
> at
jdk.test.lib.process.OutputAnalyzer.shouldMatch(OutputAnalyzer.java:306)
> at
TestMemoryAwareness.testOperatingSystemMXBeanAwareness(TestMemoryAwareness.java:151)
> at TestMemoryAwareness.main(TestMemoryAwareness.java:73)
> at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
> at
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
> at java.base/java.lang.Thread.run(Thread.java:832)
>
> Testing: Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker
tests passed. Tier4-tier6 tests are still running.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.05
> [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8226575
> [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8228428
> [4] https://bugs.openjdk.java.net/browse/JDK-8235522
>
> Thank you,
>
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
ze: %d",
osBean.getFreePhysicalMemorySize()));
40 log(String.format("OperatingSystemMXBean.getTotalSwapSpaceSize:
%d", osBean.getTotalSwapSpaceSize()));
41 log(String.format("OperatingSystemMXBean.getFreeSwapSpaceSize:
%d", osBean.getFreeSwapSpaceSize()));
42 log(String.format("OperatingSystemMXBean.getCpuLoad: %f",
osBean.getCpuLoad()));
43 log(String.format("OperatingSystemMXBean.getSystemCpuLoad: %f",
osBean.getSystemCpuLoad()));
Thanks,
Serguei
On 12/6/19 17:41, Daniil Titov wrote:
> Hi David, Mandy, and Bob,
>
> Thank you for reviewing this fix.
>
> Please review a new version of the fix [1] that includes the following
changes comparing to the previous version of the webrev ( webrev.04)
> 1. The changes in Javadoc made in the webrev.04 comparing to webrev.03
and to CSR [3] were discarded.
> 2. The implementation of methods getFreeMemorySize, getTotalMemorySize,
getFreeSwapSpaceSize and getTotalSwapSpaceSize
> was also reverted to webrev.03 version that return host's values if
the metrics are unavailable or cannot be properly read.
> I would like to mention that currently the native implementation
of these methods de-facto may return -1 at some circumstances,
> but I agree that the changes proposed in the previous version of
the webrev increase such probability.
> I filed the follow-up issue [4] as Mandy suggested.
> 3. The legacy methods were renamed as David suggested.
>
>
>> src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
>> ! static int initialized=1;
>>
>> Am I reading this right that the code currently fails to actually do
the
>> initialization because of this ???
> Yes, currently the code fails to do the initialization but it was
unnoticed since method
> get_cpuload_internal(...) was never called for a specific CPU, the first
parameter "which"
> was always -1.
>
>> test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java
>>
>> System.out.println(String.format(...)
>>
>> Why not simply
>>
>> System.out.printf(..)
> As I tried explain it earlier it would make the tests unstable.
> System.out.printf(...) just delegates the call to System.out.format(...)
that doesn't emit the string atomically.
> Instead it parses the format string into a list of FormatString objects
and then iterates over the list.
> As a result, the other traces occasionally got printed between these
iterations and break the pattern the test is expected to find
> in the output.
>
> For example, here is the sample of a such output that has the trace
message printed between " OperatingSystemMXBean.getFreePhysicalMemorySize:"
> and "1030762496".
>
>
> [0.304s][trace][os,container] Memory Usage is: 42983424
> OperatingSystemMXBean.getFreeMemorySize: 1030758400
> [0.305s][trace][os,container] Path to /memory.usage_in_bytes is
/sys/fs/cgroup/memory/memory.usage_in_bytes
> [0.305s][trace][os,container] Memory Usage is: 42979328
> [0.306s][trace][os,container] Path to /memory.usage_in_bytes is
/sys/fs/cgroup/memory/memory.usage_in_bytes
> OperatingSystemMXBean.getFreePhysicalMemorySize:
[0.306s][trace][os,container] Memory Usage is: 42975232
> 1030762496
> OperatingSystemMXBean.getTotalSwapSpaceSize: 499122176
>
>
> java.lang.RuntimeException:
'OperatingSystemMXBean\\.getFreePhysicalMemorySize: [1-9][0-9]+' missing from
stdout/stderr
>
> at
jdk.test.lib.process.OutputAnalyzer.shouldMatch(OutputAnalyzer.java:306)
> at
TestMemoryAwareness.testOperatingSystemMXBeanAwareness(TestMemoryAwareness.java:151)
> at TestMemoryAwareness.main(TestMemoryAwareness.java:73)
> at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
> at
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
> at java.base/java.lang.Thread.run(Thread.java:832)
>
> Testing: Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker
tests passed. Tier4-tier6 tests are still running.
>
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.05
> [2] Jira issue: https://bugs.openjdk.jav
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
Hi David,
> Please file a follow up RFE to look into this.
I created an issue to follow this up [1]
[1] https://bugs.openjdk.java.net/browse/JDK-8235681
Thank you,
Daniil
On 12/10/19, 2:11 AM, "David Holmes" wrote:
On 10/12/2019 5:31 am, Daniil Titov wrote:
> Hi David,
>
>> So we never try to access the uninitialized counters.cpus array
which is
>> good but we still return garbage for counters.jvmTicks and
>>counters.cpuTicks - surely that should have been noticeable?
>
> It only affected the first time the CPU load was requested. Function
get_cpuload_internal(...)
> calls get_totalticks() and get_jvmticks() functions that update these
counters.
> But on the first call, yes, it compares the newly received counters with
the garbage.
>
>
> It has the code that seems to be written to somehow mitigate it and in
worse case just return
> 0 or 1.0. But it also could be that there is some other problem this
code tries to solve so I'm not sure
> we should remove these workarounds as a part of the current fix.
Please file a follow up RFE to look into this.
Thanks,
David
> 274 // seems like we sometimes end up with less kernel ticks
when
> 275 // reading /proc/self/stat a second time, timing issue
between cpus?
> 276 if (pticks->usedKernel < tmp.usedKernel) {
> 277 kdiff = 0;
> 278 } else {
> 279 kdiff = pticks->usedKernel - tmp.usedKernel;
> 280 }
> 281 tdiff = pticks->total - tmp.total;
> 282 udiff = pticks->used - tmp.used;
> 283
> 284 if (tdiff == 0) {
> 285 user_load = 0;
> 286 } else {
> 287 if (tdiff < (udiff + kdiff)) {
> 288 tdiff = udiff + kdiff;
> 289 }
> 290 *pkernelLoad = (kdiff / (double)tdiff);
> 291 // BUG9044876, normalize return values to sane
values
> 292 *pkernelLoad = MAX(*pkernelLoad, 0.0);
> 293 *pkernelLoad = MIN(*pkernelLoad, 1.0);
> 294
> 295 user_load = (udiff / (double)tdiff);
> 296 user_load = MAX(user_load, 0.0);
> 297 user_load = MIN(user_load, 1.0);
> 298 }
> 299 }
>
> Best regards,
> Daniil
>
> On 12/8/19, 8:49 PM, "David Holmes" wrote:
>
> Hi Daniil,
>
> On 7/12/2019 11:41 am, Daniil Titov wrote:
> > Hi David, Mandy, and Bob,
> >
> > Thank you for reviewing this fix.
> >
> > Please review a new version of the fix [1] that includes the
following changes comparing to the previous version of the webrev ( webrev.04)
> > 1. The changes in Javadoc made in the webrev.04 comparing to
webrev.03 and to CSR [3] were discarded.
>
> Okay.
>
> > 2. The implementation of methods getFreeMemorySize,
getTotalMemorySize, getFreeSwapSpaceSize and getTotalSwapSpaceSize
> > was also reverted to webrev.03 version that return host's
values if the metrics are unavailable or cannot be properly read.
>
> Okay.
>
> > I would like to mention that currently the native
implementation of these methods de-facto may return -1 at some circumstances,
> > but I agree that the changes proposed in the previous
version of the webrev increase such probability.
> > I filed the follow-up issue [4] as Mandy suggested.
>
> I added a comment to the bug. This is potentially a difficult
problem to
> resolve - it all depends on the likelihood of any errors and what
they
> really indicate.
>
> > 3. The legacy methods were renamed as David suggested.
>
> Thanks!
>
> >
> >>
src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
> >> ! static int initialized=1;
> >>
> >> Am I reading this right that the code currently fails to
actually do the
> >> initialization because of this ???
> >
> > Yes, currently the code fails to do the initialization but it was
unnoticed since method
> > get_cpuload_internal(...) was never c
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
Hi Mandy and Bob,
Please review a new version of the webrev [1] that moves doPrivileged calls in
jdk.internal.platform.cgroupv1.SubSystem to separate methods that throw
IOException, as Mandy suggested.
Mach5 tests are still running.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.06/
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8226575
[3] CSR: https://bugs.openjdk.java.net/browse/JDK-8228428
Thank you,
Daniil
On 12/9/19, 1:55 PM, "Mandy Chung" wrote:
On 12/9/19 10:51 AM, Daniil Titov wrote:
> Hi Mandy and Bob,
>
>> Why did you not change the exception caught in
SubSystem.java:getStringValue to PrivilegedActionException from IOException
>> so it’s consistent with the other get functions?
> In this method both Files.newBufferedReader and return
bufferedReader.readLine could throw IOException so for simplicity I just put
> the whole code block in doPrivileged. On the other side I don't believe
that BufferedReader.readline() requires FilePermission checks ( and tests
proved that)
> so we could change this implementation to the following:
>
> public static String getStringValue(SubSystem subsystem, String
parm) {
> if (subsystem == null) return null;
>
> try (BufferedReader bufferedReader =
>
AccessController.doPrivileged((PrivilegedExceptionAction) () ->
{
> return
Files.newBufferedReader(Paths.get(subsystem.path(), parm));
> })) {
> return bufferedReader.readLine();
> } catch (PrivilegedActionException | IOException e) {
> return null;
> }
> }
>
> Could you please advise are you OK with it or you would like to proceed
with the approach Mandy suggested to unwrap
> PrivilegedActionException exception and throw the cause instead?
>
I think it's simpler to read and understand if the doPrivileged call is
moved out as a separate method that will throw IOException as the
expected functionality as suggested above.
For SubSystem::getStringValue, one suggestion would be:
diff --git
a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
---
a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
+++
b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
@@ -29,7 +29,11 @@
import java.io.IOException;
import java.math.BigInteger;
import java.nio.file.Files;
+import java.nio.file.Path;
import java.nio.file.Paths;
+import java.security.AccessController;
+import java.security.PrivilegedActionException;
+import java.security.PrivilegedExceptionAction;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
@@ -90,9 +94,8 @@
public static String getStringValue(SubSystem subsystem, String
parm) {
if (subsystem == null) return null;
-try(BufferedReader bufferedReader =
Files.newBufferedReader(Paths.get(subsystem.path(), parm))) {
-String line = bufferedReader.readLine();
-return line;
+try {
+return subsystem.readStringValue(parm);
}
catch (IOException e) {
return null;
@@ -100,6 +103,24 @@
}
+private String readStringValue(String param) throws IOException {
+PrivilegedExceptionAction pea = () ->
Files.newBufferedReader(Paths.get(path(), param));
+try (BufferedReader bufferedReader =
AccessController.doPrivileged(pea)) {
+String line = bufferedReader.readLine();
+return line;
+} catch (PrivilegedActionException e) {
+Throwable x = e.getCause();
+if (x instanceof IOException)
+throw (IOException)x;
+if (x instanceof RuntimeException)
+throw (RuntimeException)x;
+if (x instanceof Error)
+throw (Error)x;
+
+throw new InternalError(x);
+}
+}
+
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
Hi David,
> So we never try to access the uninitialized counters.cpus array which is
>good but we still return garbage for counters.jvmTicks and
> counters.cpuTicks - surely that should have been noticeable?
It only affected the first time the CPU load was requested. Function
get_cpuload_internal(...)
calls get_totalticks() and get_jvmticks() functions that update these counters.
But on the first call, yes, it compares the newly received counters with the
garbage.
It has the code that seems to be written to somehow mitigate it and in worse
case just return
0 or 1.0. But it also could be that there is some other problem this code
tries to solve so I'm not sure
we should remove these workarounds as a part of the current fix.
274 // seems like we sometimes end up with less kernel ticks when
275 // reading /proc/self/stat a second time, timing issue between
cpus?
276 if (pticks->usedKernel < tmp.usedKernel) {
277 kdiff = 0;
278 } else {
279 kdiff = pticks->usedKernel - tmp.usedKernel;
280 }
281 tdiff = pticks->total - tmp.total;
282 udiff = pticks->used - tmp.used;
283
284 if (tdiff == 0) {
285 user_load = 0;
286 } else {
287 if (tdiff < (udiff + kdiff)) {
288 tdiff = udiff + kdiff;
289 }
290 *pkernelLoad = (kdiff / (double)tdiff);
291 // BUG9044876, normalize return values to sane values
292 *pkernelLoad = MAX(*pkernelLoad, 0.0);
293 *pkernelLoad = MIN(*pkernelLoad, 1.0);
294
295 user_load = (udiff / (double)tdiff);
296 user_load = MAX(user_load, 0.0);
297 user_load = MIN(user_load, 1.0);
298 }
299 }
Best regards,
Daniil
On 12/8/19, 8:49 PM, "David Holmes" wrote:
Hi Daniil,
On 7/12/2019 11:41 am, Daniil Titov wrote:
> Hi David, Mandy, and Bob,
>
> Thank you for reviewing this fix.
>
> Please review a new version of the fix [1] that includes the following
changes comparing to the previous version of the webrev ( webrev.04)
> 1. The changes in Javadoc made in the webrev.04 comparing to webrev.03
and to CSR [3] were discarded.
Okay.
> 2. The implementation of methods getFreeMemorySize, getTotalMemorySize,
getFreeSwapSpaceSize and getTotalSwapSpaceSize
> was also reverted to webrev.03 version that return host's values if
the metrics are unavailable or cannot be properly read.
Okay.
> I would like to mention that currently the native implementation
of these methods de-facto may return -1 at some circumstances,
> but I agree that the changes proposed in the previous version of
the webrev increase such probability.
> I filed the follow-up issue [4] as Mandy suggested.
I added a comment to the bug. This is potentially a difficult problem to
resolve - it all depends on the likelihood of any errors and what they
really indicate.
> 3. The legacy methods were renamed as David suggested.
Thanks!
>
>> src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
>> ! static int initialized=1;
>>
>> Am I reading this right that the code currently fails to actually do
the
>> initialization because of this ???
>
> Yes, currently the code fails to do the initialization but it was
unnoticed since method
> get_cpuload_internal(...) was never called for a specific CPU, the first
parameter "which"
> was always -1.
So we never try to access the uninitialized counters.cpus array which is
good but we still return garbage for counters.jvmTicks and
counters.cpuTicks - surely that should have been noticeable?
>> test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java
>>
>> System.out.println(String.format(...)
>>
>> Why not simply
>>
>> System.out.printf(..)
>
> As I tried explain it earlier it would make the tests unstable.
> System.out.printf(...) just delegates the call to System.out.format(...)
that doesn't emit the string atomically.
> Instead it parses the format string into a list of FormatString objects
and then iterates over the list.
> As a result, the other traces occasionally got printed between these
iterations and break the pattern the test is expected to find
> in the output.
Sorry I missed the earlier explanation. I find it somewhat surprising
that format() works that way, but without unlimited buffering there will
