Re: RFR (trivial) 8244779: ProblemList serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java pending JDK-8244571
LGTM++ and trivial. Thanks, Serguei On 5/11/20 21:38, Igor Ignatyev wrote: LGTM -- Igor On May 11, 2020, at 9:28 PM, David Holmes wrote: This test recently starting running with -Xcheck:jni applied and is failing, so we want to problem-list it until the issue can be investigated diff -r bbdcc6741915 test/hotspot/jtreg/ProblemList.txt --- a/test/hotspot/jtreg/ProblemList.txt +++ b/test/hotspot/jtreg/ProblemList.txt @@ -108,6 +108,7 @@ serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java 8214032 generic-all serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatArrayCorrectnessTest.java 8224150 generic-all +serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java 8244571 generic-all # Thanks, David
Re: RFR (trivial) 8244779: ProblemList serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java pending JDK-8244571
Thanks Igor! David On 12/05/2020 2:38 pm, Igor Ignatyev wrote: LGTM -- Igor On May 11, 2020, at 9:28 PM, David Holmes wrote: This test recently starting running with -Xcheck:jni applied and is failing, so we want to problem-list it until the issue can be investigated diff -r bbdcc6741915 test/hotspot/jtreg/ProblemList.txt --- a/test/hotspot/jtreg/ProblemList.txt +++ b/test/hotspot/jtreg/ProblemList.txt @@ -108,6 +108,7 @@ serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java 8214032 generic-all serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatArrayCorrectnessTest.java 8224150 generic-all +serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java 8244571 generic-all # Thanks, David
Re: RFR (trivial) 8244779: ProblemList serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java pending JDK-8244571
LGTM -- Igor > On May 11, 2020, at 9:28 PM, David Holmes wrote: > > This test recently starting running with -Xcheck:jni applied and is failing, > so we want to problem-list it until the issue can be investigated > > diff -r bbdcc6741915 test/hotspot/jtreg/ProblemList.txt > --- a/test/hotspot/jtreg/ProblemList.txt > +++ b/test/hotspot/jtreg/ProblemList.txt > @@ -108,6 +108,7 @@ > > serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java > 8214032 generic-all > serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatArrayCorrectnessTest.java > 8224150 generic-all > +serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java 8244571 > generic-all > > # > > Thanks, > David
RFR (trivial) 8244779: ProblemList serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java pending JDK-8244571
This test recently starting running with -Xcheck:jni applied and is failing, so we want to problem-list it until the issue can be investigated diff -r bbdcc6741915 test/hotspot/jtreg/ProblemList.txt --- a/test/hotspot/jtreg/ProblemList.txt +++ b/test/hotspot/jtreg/ProblemList.txt @@ -108,6 +108,7 @@ serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java 8214032 generic-all serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatArrayCorrectnessTest.java 8224150 generic-all +serviceability/jvmti/HiddenClass/P/Q/HiddenClassSigTest.java 8244571 generic-all # Thanks, David
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
On 05/11/2020 17:21, serguei.spit...@oracle.com wrote:
On 5/11/20 17:17, Alex Menkov wrote:
On 05/11/2020 17:08, Yasumasa Suenaga wrote:
On 2020/05/12 9:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a
thread "_thread_blocked" VM does not need to suspend it at safepoint.
I think so, but if state would be transit to the other in
is_init_trigger(), it might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).
I already did it :)
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/
It is okay with me in general.
But as a side opinion: placing the ThreadInVM inside the loop would be
simpler and without any extra problems.
I'm ok with both ways.
Especially taking in mind that this case (when we need to restart attach
listener) is quite artificial.
--alex
Thanks,
Serguei
--alex
Thanks,
Yasumasa
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding
ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each
iteration.
Also, we do not care about performance here as it is extremely rare
case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test
timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to
make significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener
restarting:
check_socket_file function aborts socket listening and waits
while attach listener state becomes AL_NOT_INITIALIZED (it
happens when AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are
used by 2 threads (attach listener thread and signal handler
thread) without synchronization;
- added close() for listening socket on aix (before it had only
shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
On 5/11/20 17:33, Yasumasa Suenaga wrote:
On 2020/05/12 9:17, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
On 5/11/20 17:08, Yasumasa Suenaga wrote:
On 2020/05/12 9:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a
thread "_thread_blocked" VM does not need to suspend it at safepoint.
I think so, but if state would be transit to the other in
is_init_trigger(), it might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).
Then I do not see it a problem to put helper inside the loop.
In normal case, just one iteration is expected.
I'm not sure that.
The attach listener state would be transit to AL_NOT_INITIALIZED when
AttachListener::dequeue() returns NULL.
listener_cleanup() would shoutdown the socket, then accept() would be
return with error at LinuxAttachListener::dequeue() in case of Linux.
If they performs on high-spec (multi-core, and high-frequency)
machine, while loop might not be finished with one iteration.
It might be long way between shutdown() call and state transition.
I agree with you this problem is rare case, so it is not a big problem
ThreadInVM is located to inside the loop.
But I prefer to locate it to outside the loop.
I'm okay with both approaches as the differences are minor and potential
performance issue (if any) is very minor.
My normal preferences are in favor of simplicity. :)
Thanks,
Serguei
Thanks,
Yasumasa
I do not see any potential performance problem here.
Thanks,
Serguei
Thanks,
Yasumasa
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at
safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding
ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each
iteration.
Also, we do not care about performance here as it is extremely
rare case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test
timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is
closed.
I don't think this _shutdown flag helps a lot, but I don't want
to make significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener
restarting:
check_socket_file function aborts socket listening and waits
while attach listener state becomes AL_NOT_INITIALIZED (it
happens when AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are
used by 2 threads (attach listener thread and signal handler
thread) without synchronization;
- added close() for listening socket on aix (before it had only
shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Hi Alex,
On 2020/05/12 9:17, Alex Menkov wrote:
On 05/11/2020 17:08, Yasumasa Suenaga wrote:
On 2020/05/12 9:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a thread
"_thread_blocked" VM does not need to suspend it at safepoint.
I think so, but if state would be transit to the other in is_init_trigger(), it
might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).
I already did it :)
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/
I sent the reply before reading yours :)
Looks good to me.
Thanks,
Yasumasa
--alex
Thanks,
Yasumasa
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) !=
AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each iteration.
Also, we do not care about performance here as it is extremely rare case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to make
significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while attach
listener state becomes AL_NOT_INITIALIZED (it happens when
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 2 threads
(attach listener thread and signal handler thread) without synchronization;
- added close() for listening socket on aix (before it had only shutdown() for
it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
On 2020/05/12 9:17, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
On 5/11/20 17:08, Yasumasa Suenaga wrote:
On 2020/05/12 9:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a thread
"_thread_blocked" VM does not need to suspend it at safepoint.
I think so, but if state would be transit to the other in is_init_trigger(), it
might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).
Then I do not see it a problem to put helper inside the loop.
In normal case, just one iteration is expected.
I'm not sure that.
The attach listener state would be transit to AL_NOT_INITIALIZED when
AttachListener::dequeue() returns NULL.
listener_cleanup() would shoutdown the socket, then accept() would be return
with error at LinuxAttachListener::dequeue() in case of Linux.
If they performs on high-spec (multi-core, and high-frequency) machine, while
loop might not be finished with one iteration.
It might be long way between shutdown() call and state transition.
I agree with you this problem is rare case, so it is not a big problem
ThreadInVM is located to inside the loop.
But I prefer to locate it to outside the loop.
Thanks,
Yasumasa
I do not see any potential performance problem here.
Thanks,
Serguei
Thanks,
Yasumasa
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each iteration.
Also, we do not care about performance here as it is extremely rare case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to make
significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while attach
listener state becomes AL_NOT_INITIALIZED (it happens when
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 2 threads
(attach listener thread and signal handler thread) without synchronization;
- added close() for listening socket on aix (before it had only shutdown() for
it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
On 05/11/2020 17:08, Yasumasa Suenaga wrote:
On 2020/05/12 9:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a
thread "_thread_blocked" VM does not need to suspend it at safepoint.
I think so, but if state would be transit to the other in
is_init_trigger(), it might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).
I already did it :)
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/
--alex
Thanks,
Yasumasa
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) !=
AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each
iteration.
Also, we do not care about performance here as it is extremely rare
case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to
make significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener
restarting:
check_socket_file function aborts socket listening and waits while
attach listener state becomes AL_NOT_INITIALIZED (it happens when
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are used
by 2 threads (attach listener thread and signal handler thread)
without synchronization;
- added close() for listening socket on aix (before it had only
shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
On 5/11/20 17:17, Alex Menkov wrote:
On 05/11/2020 17:08, Yasumasa Suenaga wrote:
On 2020/05/12 9:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a
thread "_thread_blocked" VM does not need to suspend it at safepoint.
I think so, but if state would be transit to the other in
is_init_trigger(), it might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).
I already did it :)
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/
It is okay with me in general.
But as a side opinion: placing the ThreadInVM inside the loop would be
simpler and without any extra problems.
Thanks,
Serguei
--alex
Thanks,
Yasumasa
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding
ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each
iteration.
Also, we do not care about performance here as it is extremely rare
case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test
timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to
make significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener
restarting:
check_socket_file function aborts socket listening and waits
while attach listener state becomes AL_NOT_INITIALIZED (it
happens when AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are
used by 2 threads (attach listener thread and signal handler
thread) without synchronization;
- added close() for listening socket on aix (before it had only
shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Hi Yasumasa,
On 5/11/20 17:08, Yasumasa Suenaga wrote:
On 2020/05/12 9:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a
thread "_thread_blocked" VM does not need to suspend it at safepoint.
I think so, but if state would be transit to the other in
is_init_trigger(), it might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).
Then I do not see it a problem to put helper inside the loop.
In normal case, just one iteration is expected.
I do not see any potential performance problem here.
Thanks,
Serguei
Thanks,
Yasumasa
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each
iteration.
Also, we do not care about performance here as it is extremely rare
case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test
timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to
make significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener
restarting:
check_socket_file function aborts socket listening and waits
while attach listener state becomes AL_NOT_INITIALIZED (it
happens when AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are
used by 2 threads (attach listener thread and signal handler
thread) without synchronization;
- added close() for listening socket on aix (before it had only
shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Updated webrev: http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.3/ --alex On 05/11/2020 14:12, Alex Menkov wrote: Please hold on with the review. webrev.2 causes LingeredApp crash. Looks like need to decrease ThreadBlockInVM scope. Will send new version after completing test run. On 05/11/2020 13:52, Chris Plummer wrote: 228 // If the app hangs, we don't want to wait for the to test timeout. Sorry, there was a typo in my suggestion. Should be "test to", not "to test". Oh, I didn't pay enough attention to comment updates :) --alex thanks, Chris On 5/11/20 12:53 PM, Alex Menkov wrote: Hi Yasumasa, Serguei, Chris, Thank you for the feedback updated webrev (all suggestions are applied): http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ On 05/11/2020 00:31, serguei.spit...@oracle.com wrote: Hi Alex, It looks good in general. I have a couple of minor comments. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html + // if for a reason the app hangs, we don't want to wait test timeout Nit: replace: wait test timeout => wait for test timeout I hope, you remember about copyright comments update. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html Q1: How useful is this variable? : AixAttachListener::_shutdown = false; Why is it needed on aix but not on other platforms? IFAIU AIX has issue with accept() - it hangs if the socket is closed. I don't think this _shutdown flag helps a lot, but I don't want to make significant changes in the AIX code as I cannot test it. --alex Thanks, Serguei On 5/8/20 18:14, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8235211 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ Test failures are caused by deadlock during attach listener restarting: check_socket_file function aborts socket listening and waits while attach listener state becomes AL_NOT_INITIALIZED (it happens when AttachListener::dequeue returns NULL). AttachListener::dequeue method is blocked in ThreadBlockInVM dtor. To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file. Other changes: - made _listener (and _shutdown for aix) volatile as they are used by 2 threads (attach listener thread and signal handler thread) without synchronization; - added close() for listening socket on aix (before it had only shutdown() for it); - additional logging and some cleanup in the test; - added handling of LingeredApp hang. --alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
On 2020/05/12 9:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a thread
"_thread_blocked" VM does not need to suspend it at safepoint.
I think so, but if state would be transit to the other in is_init_trigger(), it
might be affect to incorrect behavior.
So I suggested to wrap ThreadInVM and while loop with code block ({}).
Thanks,
Yasumasa
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) !=
AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each iteration.
Also, we do not care about performance here as it is extremely rare case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to make
significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while attach
listener state becomes AL_NOT_INITIALIZED (it happens when
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by 2 threads
(attach listener thread and signal handler thread) without synchronization;
- added close() for listening socket on aix (before it had only shutdown() for
it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Hi Alex,
I see the point, thanks.
Serguei
On 5/11/20 17:01, Alex Menkov wrote:
Hi Serguei,
If I understand correctly, this also should work - when we mark a
thread "_thread_blocked" VM does not need to suspend it at safepoint.
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each
iteration.
Also, we do not care about performance here as it is extremely rare
case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to
make significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener
restarting:
check_socket_file function aborts socket listening and waits while
attach listener state becomes AL_NOT_INITIALIZED (it happens when
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are used
by 2 threads (attach listener thread and signal handler thread)
without synchronization;
- added close() for listening socket on aix (before it had only
shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Hi Serguei,
If I understand correctly, this also should work - when we mark a thread
"_thread_blocked" VM does not need to suspend it at safepoint.
--alex
On 05/11/2020 15:36, serguei.spit...@oracle.com wrote:
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) !=
AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at each
iteration.
Also, we do not care about performance here as it is extremely rare case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is closed.
I don't think this _shutdown flag helps a lot, but I don't want to
make significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while
attach listener state becomes AL_NOT_INITIALIZED (it happens when
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are used
by 2 threads (attach listener thread and signal handler thread)
without synchronization;
- added close() for listening socket on aix (before it had only
shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Hi Alex,
I'm not sure this update suggested by Yasumasa is right:
536 // avoid deadlock if AttachListener thread is blocked at safepoint
537 ThreadBlockInVM tbivm(JavaThread::current());
538 while (AttachListener::transit_state(AL_INITIALIZING,
539 AL_NOT_INITIALIZED) != AL_NOT_INITIALIZED) {
540 os::naked_yield();
541 }
The synchronization window becomes smaller with adding ThreadBlockInVM,
so we probably do not see the deadlock anymore.
However, I think, we still need it inside the loop to work at
each iteration.
Also, we do not care about performance here as it is extremely
rare case.
Thanks,
Serguei
On 5/11/20 12:53, Alex Menkov wrote:
Hi
Yasumasa, Serguei, Chris,
Thank you for the feedback
updated webrev (all suggestions are applied):
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/
On 05/11/2020 00:31, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
I have a couple of minor comments.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
+ // if for a reason the app hangs, we don't want to wait test
timeout
Nit: replace: wait test timeout => wait for test timeout
I hope, you remember about copyright comments update.
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html
Q1: How useful is this variable? :
AixAttachListener::_shutdown = false;
Why is it needed on aix but not on other platforms?
IFAIU AIX has issue with accept() - it hangs if the socket is
closed.
I don't think this _shutdown flag helps a lot, but I don't want to
make significant changes in the AIX code as I cannot test it.
--alex
Thanks,
Serguei
On 5/8/20 18:14, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener
restarting:
check_socket_file function aborts socket listening and waits
while attach listener state becomes AL_NOT_INITIALIZED (it
happens when AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM
dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are
used by 2 threads (attach listener thread and signal handler
thread) without synchronization;
- added close() for listening socket on aix (before it had
only shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info
Hi Alex, LGTM Thank you for the update! Serguei On 5/11/20 14:14, Alex Menkov wrote: Hi Serguei, Alan, Updated webrev: http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.2/ --alex On 05/11/2020 11:52, Alan Bateman wrote: On 11/05/2020 19:21, serguei.spit...@oracle.com wrote: Hi Alex, There is no need to repeat this: "deploy applications thatpackage an agent with the application, or use tools that load agents into a running application" I'd suggest to rephrase it to something like: "Agents can transform classes in arbitrary ways at load time, transform modules, or transform the bytecode of methods of already loaded classes. Developers or administrators that deploy agents are responsible for their trustworthiness and must therefore verify each agent including the content and structure of its JAR file." Also, could you, please, replace: * The three ways to start an agent is described below. with: * The three ways to start an agent are described below. Serguei's suggestions look good. -Alan
Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info
Hi Serguei, Alan, Updated webrev: http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.2/ --alex On 05/11/2020 11:52, Alan Bateman wrote: On 11/05/2020 19:21, serguei.spit...@oracle.com wrote: Hi Alex, There is no need to repeat this: "deploy applications thatpackage an agent with the application, or use tools that load agents into a running application" I'd suggest to rephrase it to something like: "Agents can transform classes in arbitrary ways at load time, transform modules, or transform the bytecode of methods of already loaded classes. Developers or administrators that deploy agents are responsible for their trustworthiness and must therefore verify each agent including the content and structure of its JAR file." Also, could you, please, replace: * The three ways to start an agent is described below. with: * The three ways to start an agent are described below. Serguei's suggestions look good. -Alan
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Please hold on with the review. webrev.2 causes LingeredApp crash. Looks like need to decrease ThreadBlockInVM scope. Will send new version after completing test run. On 05/11/2020 13:52, Chris Plummer wrote: 228 // If the app hangs, we don't want to wait for the to test timeout. Sorry, there was a typo in my suggestion. Should be "test to", not "to test". Oh, I didn't pay enough attention to comment updates :) --alex thanks, Chris On 5/11/20 12:53 PM, Alex Menkov wrote: Hi Yasumasa, Serguei, Chris, Thank you for the feedback updated webrev (all suggestions are applied): http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ On 05/11/2020 00:31, serguei.spit...@oracle.com wrote: Hi Alex, It looks good in general. I have a couple of minor comments. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html + // if for a reason the app hangs, we don't want to wait test timeout Nit: replace: wait test timeout => wait for test timeout I hope, you remember about copyright comments update. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html Q1: How useful is this variable? : AixAttachListener::_shutdown = false; Why is it needed on aix but not on other platforms? IFAIU AIX has issue with accept() - it hangs if the socket is closed. I don't think this _shutdown flag helps a lot, but I don't want to make significant changes in the AIX code as I cannot test it. --alex Thanks, Serguei On 5/8/20 18:14, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8235211 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ Test failures are caused by deadlock during attach listener restarting: check_socket_file function aborts socket listening and waits while attach listener state becomes AL_NOT_INITIALIZED (it happens when AttachListener::dequeue returns NULL). AttachListener::dequeue method is blocked in ThreadBlockInVM dtor. To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file. Other changes: - made _listener (and _shutdown for aix) volatile as they are used by 2 threads (attach listener thread and signal handler thread) without synchronization; - added close() for listening socket on aix (before it had only shutdown() for it); - additional logging and some cleanup in the test; - added handling of LingeredApp hang. --alex
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
228 // If the app hangs, we don't want to wait for the to test timeout. Sorry, there was a typo in my suggestion. Should be "test to", not "to test". thanks, Chris On 5/11/20 12:53 PM, Alex Menkov wrote: Hi Yasumasa, Serguei, Chris, Thank you for the feedback updated webrev (all suggestions are applied): http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ On 05/11/2020 00:31, serguei.spit...@oracle.com wrote: Hi Alex, It looks good in general. I have a couple of minor comments. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html + // if for a reason the app hangs, we don't want to wait test timeout Nit: replace: wait test timeout => wait for test timeout I hope, you remember about copyright comments update. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html Q1: How useful is this variable? : AixAttachListener::_shutdown = false; Why is it needed on aix but not on other platforms? IFAIU AIX has issue with accept() - it hangs if the socket is closed. I don't think this _shutdown flag helps a lot, but I don't want to make significant changes in the AIX code as I cannot test it. --alex Thanks, Serguei On 5/8/20 18:14, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8235211 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ Test failures are caused by deadlock during attach listener restarting: check_socket_file function aborts socket listening and waits while attach listener state becomes AL_NOT_INITIALIZED (it happens when AttachListener::dequeue returns NULL). AttachListener::dequeue method is blocked in ThreadBlockInVM dtor. To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file. Other changes: - made _listener (and _shutdown for aix) volatile as they are used by 2 threads (attach listener thread and signal handler thread) without synchronization; - added close() for listening socket on aix (before it had only shutdown() for it); - additional logging and some cleanup in the test; - added handling of LingeredApp hang. --alex
Re: RFR: 8242009: Review setting test.java/vm.opts in jcmd/jhsdb and debugger in serviceability tests
BTW, I just found out that due to a fix for JDK-8220295 early last year, the jdk/sun/tools tests are not run in parallel, so at least atm are not contributing to any host memory related issues. Still it is best to fix them now. Chris On 5/11/20 10:54 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 >> >> 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.
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Hi Yasumasa, Serguei, Chris, Thank you for the feedback updated webrev (all suggestions are applied): http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev.2/ On 05/11/2020 00:31, serguei.spit...@oracle.com wrote: Hi Alex, It looks good in general. I have a couple of minor comments. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html + // if for a reason the app hangs, we don't want to wait test timeout Nit: replace: wait test timeout => wait for test timeout I hope, you remember about copyright comments update. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html Q1: How useful is this variable? : AixAttachListener::_shutdown = false; Why is it needed on aix but not on other platforms? IFAIU AIX has issue with accept() - it hangs if the socket is closed. I don't think this _shutdown flag helps a lot, but I don't want to make significant changes in the AIX code as I cannot test it. --alex Thanks, Serguei On 5/8/20 18:14, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8235211 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ Test failures are caused by deadlock during attach listener restarting: check_socket_file function aborts socket listening and waits while attach listener state becomes AL_NOT_INITIALIZED (it happens when AttachListener::dequeue returns NULL). AttachListener::dequeue method is blocked in ThreadBlockInVM dtor. To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file. Other changes: - made _listener (and _shutdown for aix) volatile as they are used by 2 threads (attach listener thread and signal handler thread) without synchronization; - added close() for listening socket on aix (before it had only shutdown() for it); - additional logging and some cleanup in the test; - added handling of LingeredApp hang. --alex
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Daniil, In the grand scheme of things, could servicability use the signature parsing support in HotSpot? Thanks, Roger On 5/9/20 12:29 PM, 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: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
I do not see my message reached the serviceability-dev mailing list. Resending... On 5/11/20 00:31, serguei.spit...@oracle.com wrote: Hi Alex, It looks good in general. I have a couple of minor comments. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html +// if for a reason the app hangs, we don't want to wait test timeout Nit: replace: wait test timeout => wait for test timeout I hope, you remember about copyright comments update. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html Q1: How useful is this variable? : AixAttachListener::_shutdown = false; Why is it needed on aix but not on other platforms? Thanks, Serguei On 5/8/20 18:14, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8235211 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ Test failures are caused by deadlock during attach listener restarting: check_socket_file function aborts socket listening and waits while attach listener state becomes AL_NOT_INITIALIZED (it happens when AttachListener::dequeue returns NULL). AttachListener::dequeue method is blocked in ThreadBlockInVM dtor. To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file. Other changes: - made _listener (and _shutdown for aix) volatile as they are used by 2 threads (attach listener thread and signal handler thread) without synchronization; - added close() for listening socket on aix (before it had only shutdown() for it); - additional logging and some cleanup in the test; - added handling of LingeredApp hang. --alex
Re: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info
On 11/05/2020 19:21, serguei.spit...@oracle.com wrote: Hi Alex, There is no need to repeat this: "deploy applications thatpackage an agent with the application, or use tools that load agents into a running application" I'd suggest to rephrase it to something like: "Agents can transform classes in arbitrary ways at load time, transform modules, or transform the bytecode of methods of already loaded classes. Developers or administrators that deploy agents are responsible for their trustworthiness and must therefore verify each agent including the content and structure of its JAR file." Also, could you, please, replace: * The three ways to start an agent is described below. with: * The three ways to start an agent are described below. Serguei's suggestions look good. -Alan
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: Ping: RFR: JDK-8243012: Fix issues in j.l.i package info
Hi Alex, There is no need to repeat this: "deploy applications that package an agent with the application, or use tools that load agents into a running application" I'd suggest to rephrase it to something like: "Agents can transform classes in arbitrary ways at load time, transform modules, or transform the bytecode of methods of already loaded classes. Developers or administrators that deploy agents are responsible for their trustworthiness and must therefore verify each agent including the content and structure of its JAR file." Also, could you, please, replace: * The three ways to start an agent is described below. with: * The three ways to start an agent are described below. Thanks, Serguei On 5/7/20 18:19, Alex Menkov wrote: On 05/01/2020 15:22, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8243012 The change fixes security note in the java.lang.instrument javadoc. webrev: http://cr.openjdk.java.net/~amenkov/jdk15/java_instrument_spec/webrev.1/ --alex
Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
> On May 8, 2020, at 12:48 PM, Daniel D. Daugherty
> wrote:
>
> On 5/7/20 1:35 AM, Mikael Vidstedt wrote:
>> New webrev here:
>>
>> webrev:
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/
>
> This pretty much says it all:
>
> > Summary of changes:90904 lines changed: 8 ins; 90725 del; 171 mod;
> > 103780 unchg
>
>
> My review is focused on looking at the changes and not looking for missed
> changes. I figure there's enough work here just looking at the changes to
> keep me occupied for a while and enough people have posted comments about
> finding other things to be examined, etc...
>
> Unlike my normal reviews, I won't be listing all the touched files;
> (there's _only_ 427 of them...)
>
> Don't forget to make a copyright year update pass before you push.
Yup - I have added it in 10 different places on my TODO list to maximize the
likelihood of me remembering it :)
>
> src/hotspot/os/posix/os_posix.hpp
> L174
> old L175 #ifndef SOLARIS
> L176
>nit - on most of this style of deletion you also got rid of
>one of the blank lines, but not here.
Oops, will fix.
> src/hotspot/share/utilities/dtrace.hpp
> old L42: #elif defined(__APPLE__)
> old L44: #include
> old L45: #else
> new L32: #include
> was previous included only for __APPLE__ and it
> is now there for every platform. Any particular reason?
No particular reason other than "it looks cleaner". I guess we could see if the
include can be removed altogether.
> Thumbs up!
Thanks for the review!!
Cheers,
Mikael
>
>> incremental:
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot.incr/open/webrev/
>>
>> Remaining items:
>>
>> * File follow-up to remove STACK_BIAS
>>
>> * File follow-ups to change/update/remove flags and/or flag documentation:
>> UseLWPSynchronization, BranchOnRegister, LIRFillDelaySlots,
>> ArrayAllocatorMallocLimit, ThreadPriorityPolicy
>>
>> * File follow-up(s) to update comments ("solaris", “sparc”, “solstudio”,
>> “sunos”, “sun studio”, “s compiler bug”, “niagara”, …)
>>
>>
>> Please let me know if there’s something I have missed!
>>
>> Cheers,
>> Mikael
>>
>>> On May 3, 2020, at 10:12 PM, Mikael Vidstedt
>>> wrote:
>>>
>>>
>>> Please review this change which implements part of JEP 381:
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>> webrev:
>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>>>
>>>
>>> Note: When reviewing this, please be aware that this exercise was
>>> *extremely* mind-numbing, so I appreciate your help reviewing all the
>>> individual changes carefully. You may want to get that coffee cup filled up
>>> (or whatever keeps you awake)!
>>>
>>>
>>> Background:
>>>
>>> Because of the size of the total patch and wide range of areas touched,
>>> this patch is one out of in total six partial patches which together make
>>> up the necessary changes to remove the Solaris and SPARC ports. The other
>>> patches are being sent out for review to mailing lists appropriate for the
>>> respective areas the touch. An email will be sent to jdk-dev summarizing
>>> all the patches/reviews. To be clear: this patch is *not* in itself
>>> complete and stand-alone - all of the (six) patches are needed to form a
>>> complete patch. Some changes in this patch may look wrong or incomplete
>>> unless also looking at the corresponding changes in other areas.
>>>
>>> For convenience, I’m including a link below[1] to the full webrev, but in
>>> case you have comments on changes in other areas, outside of the files
>>> included in this thread, please provide those comments directly in the
>>> thread on the appropriate mailing list for that area if possible.
>>>
>>> In case it helps, the changes were effectively produced by searching for
>>> and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”,
>>> etc. More information about the areas impacted can be found in the JEP
>>> itself.
>>>
>>> A big thank you to Igor Ignatyev for helping make the changes to the
>>> hotspot tests!
>>>
>>> Also, I have a short list of follow-ups which I’m going to look at
>>> separately from this JEP/patch, mainly related to command line
>>> options/flags which are no longer relevant and should be
>>> deprecated/obsoleted/removed.
>>>
>>> Testing:
>>>
>>> A slightly earlier version of this change successfully passed tier1-8, as
>>> well as client tier1-2. Additional testing will be done after the first
>>> round of reviews has been completed.
>>>
>>> Cheers,
>>> Mikael
>>>
>>> [1]
>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
>>>
>
Re: RFR: 8241080: Consolidate signature parsing code in serviceability tools
Hi Daniil, Overall looks good. Just one minor thing. In ClassTypeImpl.c and ObjectReferenceImpl.c I think the following would be more readable with an if/else: 79 return JNI_FUNC_PTR(env,ExceptionOccurred)(env) ? AGENT_ERROR_JNI_EXCEPTION 80 : JVMTI_ERROR_NONE; Also, have you filed a CR for the JDWP spec issues related to missing type information in some of the protocol packet descriptions? thanks, Chris On 5/9/20 9:29 AM, 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 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 >> >> 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 >> > >>
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Hi Alex,
228 // if for a reason the app hangs, we don't want to
wait test timeout
229 if
(!appProcess.waitFor(Utils.adjustTimeout(appWaitTime), TimeUnit.SECONDS)) {
Can you fix the comment. Maybe:
228 // If the app hangs, we don't want to wait for the to
test timeout.
And your use of appWaitTime had me look for other uses, and I noticed a
pre-existing comment there that could use some work. Perhaps you can
clean it up with this RFR.
273 * Waits the application to start with the default timeout.
Should be "Waits for the application..."
thanks,
Chris
On 5/8/20 6:14 PM, Alex Menkov wrote:
Hi all,
please review the fix for
https://bugs.openjdk.java.net/browse/JDK-8235211
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/
Test failures are caused by deadlock during attach listener restarting:
check_socket_file function aborts socket listening and waits while
attach listener state becomes AL_NOT_INITIALIZED (it happens when
AttachListener::dequeue returns NULL).
AttachListener::dequeue method is blocked in ThreadBlockInVM dtor.
To solve it ThreadBlockInVM was added inside waiting cycle in
check_socket_file.
Other changes:
- made _listener (and _shutdown for aix) volatile as they are used by
2 threads (attach listener thread and signal handler thread) without
synchronization;
- added close() for listening socket on aix (before it had only
shutdown() for it);
- additional logging and some cleanup in the test;
- added handling of LingeredApp hang.
--alex
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/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
Re: RFR 8244105: JDK 11.0.7 -Xlog gc issue with file path if not exist
Hi, I think it would be very convenient for app developers if JVM were able to create intermediate directories to gc.log file if they do not exist. I.e. $ if [ -f logs/gc.log ]; then echo "log file exists"; else echo "log file does not exist"; fi log file does not exist $ if [ -d logs ]; then echo "log directory exists"; else echo "log directory does not exist"; fi log directory does not exist $ java -Xlog:"gc*:file=logs/gc.log" -version ... $ if [ -d logs ]; then echo "log directory exists"; else echo "log directory does not exist"; fi log directory exists $ if [ -f logs/gc.log ]; then echo "log file exists"; else echo "log file does not exist"; fi log file exists Thanks, Dmytro On Fri, May 8, 2020 at 8:31 PM Harold Seigel wrote: > Hi, > > Please review this fix for JDK-8244105. The fix continues program > execution even when specified logging options are invalid. Previously, > invalid logging options terminated the program. Now, a warning is issued. > For example: > > > java -Xlog:"gc*:file=/dont/exist" -version > [0.001s][error][logging] Error opening log file '/dont/exist': No such > file or directory > [0.001s][error][logging] Initialization of output 'file=/dont/exist' using > options '(null)' failed. > Java HotSpot(TM) 64-Bit Server VM warning: Invalid -Xlog option > '-Xlog:gc*:file=/dont/exist', see error log for details. > > java version "15-internal" 2020-09-15 > Java(TM) SE Runtime Environment (fastdebug build > 15-internal+0-2020-05-08-1313404.hseigel.bug8244105) > Java HotSpot(TM) 64-Bit Server VM (fastdebug build > 15-internal+0-2020-05-08-1313404.hseigel.bug8244105, mixed mode, sharing) > > Open Webrev: > http://cr.openjdk.java.net/~hseigel/bug_8244105/webrev/index.html > > JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8244105 > > The fix was regression tested by running Mach5 tiers 1 and 2 tests and > builds on Linux-x64, Solaris, Windows, and Mac OS X and by running Mach5 > tiers 3-5 tests on Linux-x64. > > Thanks, Harold > > >
Re: RFR: JDK-8235211: serviceability/attach/RemovingUnixDomainSocketTest.java fails with AttachNotSupportedException: Unable to open socket file
Hi Alex, It looks good in general. I have a couple of minor comments. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html +// if for a reason the app hangs, we don't want to wait test timeout Nit: replace: wait test timeout => wait for test timeout I hope, you remember about copyright comments update. http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/src/hotspot/os/aix/attachListener_aix.cpp.udiff.html Q1: How useful is this variable? : AixAttachListener::_shutdown = false; Why is it needed on aix but not on other platforms? Thanks, Serguei On 5/8/20 18:14, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8235211 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/RemovingUnixDomainSocket/webrev/ Test failures are caused by deadlock during attach listener restarting: check_socket_file function aborts socket listening and waits while attach listener state becomes AL_NOT_INITIALIZED (it happens when AttachListener::dequeue returns NULL). AttachListener::dequeue method is blocked in ThreadBlockInVM dtor. To solve it ThreadBlockInVM was added inside waiting cycle in check_socket_file. Other changes: - made _listener (and _shutdown for aix) volatile as they are used by 2 threads (attach listener thread and signal handler thread) without synchronization; - added close() for listening socket on aix (before it had only shutdown() for it); - additional logging and some cleanup in the test; - added handling of LingeredApp hang. --alex
