[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: [PATCH] 7142035 assert in j.l.instrument agents during shutdown when daemon thread is running
From: Staffan Larsen <staffan.larsen () oracle ! com>
Date: 2014-01-29 11:34:40
Message-ID: 26CA95D5-95E7-447F-81E9-6E2BD7CA2A7F () oracle ! com
[Download RAW message or body]
The patch has been pushed: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/87fdc579dafc
Thanks for contributing!
/Staffan
On 28 jan 2014, at 15:59, Chan, Sunny <Sunny.Chan@gs.com> wrote:
> Hi Staffan,
>
> I agreed with your assessment and have incorporated your extra change into my \
> patch. I have a couple of test runs and have determined that we can reduce each \
> process period but we will need to repeat at least 50 times to reliably reproduce \
> the issue in unpatched builds. So the intervals is now 200ms instead of 1 second.
> Let me know if there is anything else need to change.
>
> Thanks
> Sunny
>
> From: Staffan Larsen [mailto:staffan.larsen@oracle.com]
> Sent: 27 January 2014 16:14
> To: Chan, Sunny [Tech]
> Cc: serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net
> Subject: Re: [PATCH] 7142035 assert in j.l.instrument agents during shutdown when \
> daemon thread is running
> Hi Sunny,
>
> This looks good!
>
> - I’d like to avoid bug ids in the names of tests, so can you change the directory \
> name to say “DaemonThread”?
> - The test takes quite a long time to run (~1 minute). I wonder if we should \
> shorten that by either starting less processes (currently 50) or letting each \
> process run for a shorter period (currently ~1 sec).
> - I think we can reduce the noise by removing the System.out.println() output from \
> the DummyAgent.
> - During one of my test runs I ran into the following failure:
> *** java.lang.instrument ASSERTION FAILED ***: "error == JVMTI_ERROR_NONE" at \
> Reentrancy.c line: 133 It looks like the patch isn’t complete. I added the patch \
> below and have not seen a failure after that. Can you incorporate that change as \
> well (if you agree with it)?
> Thanks,
> /Staffan
>
>
> --- a/src/share/instrument/Reentrancy.c
> +++ b/src/share/instrument/Reentrancy.c
> @@ -130,6 +130,7 @@
> error = confirmingTLSSet ( jvmtienv,
> thread,
> JPLIS_CURRENTLY_INSIDE_TOKEN);
> + check_phase_ret_false(error);
> jplis_assert(error == JVMTI_ERROR_NONE);
> if ( error != JVMTI_ERROR_NONE ) {
> result = JNI_FALSE;
>
>
> On 27 jan 2014, at 12:44, Chan, Sunny <Sunny.Chan@gs.com> wrote:
>
>
> Hi Staffan,
>
> I have attached the new version of the patch - I have reworked the test case and \
> now it is mostly based in Java, but I have decided to keep using the shell script \
> to build the Agent Jar file as it is easier.
> Thanks.
>
> From: Staffan Larsen [mailto:staffan.larsen@oracle.com]
> Sent: 13 January 2014 12:37
> To: Chan, Sunny [Tech]
> Cc: serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net
> Subject: Re: [PATCH] 7142035 assert in j.l.instrument agents during shutdown when \
> daemon thread is running
>
> On 8 jan 2014, at 16:48, Chan, Sunny <Sunny.Chan@gs.com> wrote:
>
>
>
> In terms of the bug fix itself does it look fine?
>
> Yes, it does.
>
> Thanks,
> /Staffan
> <7142035rev1.patch>
>
> <7142035rev2.patch>
[Attachment #3 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html \
charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: \
space; -webkit-line-break: after-white-space;">The patch has been pushed: <a \
href="http://hg.openjdk.java.net/jdk9/dev/jdk/rev/87fdc579dafc">http://hg.openjdk.java.net/jdk9/dev/jdk/rev/87fdc579dafc</a><div><br></div><div>Thanks \
for contributing!</div><div><br></div><div>/Staffan</div><div><br></div><div><div><div>On \
28 jan 2014, at 15:59, Chan, Sunny <<a \
href="mailto:Sunny.Chan@gs.com">Sunny.Chan@gs.com</a>> wrote:</div><br \
class="Apple-interchange-newline"><blockquote type="cite"><div lang="EN-GB" \
link="blue" vlink="purple" style="font-family: Helvetica; font-size: 16px; \
font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: \
normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; \
text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; \
-webkit-text-stroke-width: 0px;"><div class="WordSection1" style="page: \
WordSection1;"><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: \
'Times New Roman', serif;"><span style="font-size: 11pt; font-family: Calibri, \
sans-serif; color: rgb(31, 73, 125);">Hi Staffan,<o:p></o:p></span></div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: \
rgb(31, 73, 125);"> </span></div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: \
11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">I agreed with your \
assessment and have incorporated your extra change into my patch. I have a couple of \
test runs and have determined that we can reduce each process period but we will need \
to repeat at least 50 times to reliably reproduce the issue in unpatched builds. So \
the intervals is now 200ms instead of 1 second.<o:p></o:p></span></div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: \
rgb(31, 73, 125);"> </span></div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: \
11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">Let me know if \
there is anything else need to change.<o:p></o:p></span></div><div style="margin: 0cm \
0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span \
style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, \
125);"> </span></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; \
font-family: 'Times New Roman', serif;"><span style="font-size: 11pt; font-family: \
Calibri, sans-serif; color: rgb(31, 73, 125);">Thanks<o:p></o:p></span></div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: \
rgb(31, 73, 125);">Sunny<o:p></o:p></span></div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: \
11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, \
125);"> </span></div><div><div style="border-style: solid none none; \
border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0cm \
0cm;"><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New \
Roman', serif;"><b><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, \
sans-serif;">From:</span></b><span lang="EN-US" style="font-size: 10pt; font-family: \
Tahoma, sans-serif;"><span class="Apple-converted-space"> </span>Staffan Larsen \
[<a href="mailto:staffan.larsen@oracle.com">mailto:staffan.larsen@oracle.com</a>]<span \
class="Apple-converted-space"> </span><br><b>Sent:</b><span \
class="Apple-converted-space"> </span>27 January 2014 16:14<br><b>To:</b><span \
class="Apple-converted-space"> </span>Chan, Sunny [Tech]<br><b>Cc:</b><span \
class="Apple-converted-space"> </span><a \
href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a> \
<a href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a><br><b>Subject:</b><span \
class="Apple-converted-space"> </span>Re: [PATCH] 7142035 assert in \
j.l.instrument agents during shutdown when daemon thread is \
running<o:p></o:p></span></div></div></div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;">Hi Sunny,<o:p></o:p></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;">This looks \
good!<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: \
12pt; font-family: 'Times New Roman', serif;"><o:p> </o:p></div></div><div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;">- I’d like to avoid bug ids in the names of tests, so can you change the \
directory name to say “DaemonThread”?<o:p></o:p></div></div><div><div style="margin: \
0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;">- The test takes quite a \
long time to run (~1 minute). I wonder if we should shorten that by either starting \
less processes (currently 50) or letting each process run for a shorter period \
(currently ~1 sec).<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;">- I think we can reduce the \
noise by removing the System.out.println() output from the \
DummyAgent.<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;">- During one of my test runs \
I ran into the following failure:<o:p></o:p></div></div><div><div style="margin: 0cm \
0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">*** \
java.lang.instrument ASSERTION FAILED ***: "error == JVMTI_ERROR_NONE" at \
Reentrancy.c line: 133<o:p></o:p></div></div><div><div style="margin: 0cm 0cm \
0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">It looks like the \
patch isn’t complete. I added the patch below and have not seen a failure after that. \
Can you incorporate that change as well (if you agree with \
it)? <o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div></div><div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;">Thanks,<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;">/Staffan<o:p></o:p></div></div></div><div><div style="margin: 0cm 0cm \
0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div></div><div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;">--- \
a/src/share/instrument/Reentrancy.c<o:p></o:p></div></div><div><div style="margin: \
0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">+++ \
b/src/share/instrument/Reentrancy.c<o:p></o:p></div></div><div><div style="margin: \
0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">@@ -130,6 \
+130,7 @@<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: \
12pt; font-family: 'Times New Roman', serif;"> \
error = confirmingTLSSet ( \
jvmtienv,<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"> \
\
thread,<o:p></o:p></div></div><div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"> \
\
JPLIS_CURRENTLY_INSIDE_TOKEN);<o:p></o:p></div></div><div><div style="margin: \
0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;">+ \
\
check_phase_ret_false(error);<o:p></o:p></div></div><div><div style="margin: \
0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"> \
jplis_assert(error == \
JVMTI_ERROR_NONE);<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"> \
if ( error != JVMTI_ERROR_NONE ) \
{<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; \
font-family: 'Times New Roman', serif;"> \
result = JNI_FALSE;<o:p></o:p></div></div></div><div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"><o:p> </o:p></div><div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;">On 27 jan 2014, at 12:44, \
Chan, Sunny <<a href="mailto:Sunny.Chan@gs.com" style="color: purple; \
text-decoration: underline;">Sunny.Chan@gs.com</a>> \
wrote:<o:p></o:p></div></div><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; \
font-family: 'Times New Roman', serif;"><br><br><o:p></o:p></div><div><div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"><span style="font-size: 11pt; font-family: Calibri, sans-serif; color: \
rgb(31, 73, 125);">Hi Staffan,</span><o:p></o:p></div></div><div><div style="margin: \
0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span \
style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, \
125);"> </span><o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: \
11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, 125);">I have attached the \
new version of the patch - I have reworked the test case and now it is mostly based \
in Java, but I have decided to keep using the shell script to build the Agent Jar \
file as it is easier.</span><o:p></o:p></div></div><div><div style="margin: 0cm 0cm \
0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span \
style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, \
125);"> </span><o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-size: \
11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, \
125);">Thanks.</span><o:p></o:p></div></div><div><div style="margin: 0cm 0cm \
0.0001pt; font-size: 12pt; font-family: 'Times New Roman', serif;"><span \
style="font-size: 11pt; font-family: Calibri, sans-serif; color: rgb(31, 73, \
125);"> </span><o:p></o:p></div></div><div><div style="border-style: solid none \
none; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0cm \
0cm;"><div style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New \
Roman', serif;"><b><span lang="EN-US" style="font-size: 10pt; font-family: Tahoma, \
sans-serif;">From:</span></b><span class="apple-converted-space"><span lang="EN-US" \
style="font-size: 10pt; font-family: Tahoma, sans-serif;"> </span></span><span \
lang="EN-US" style="font-size: 10pt; font-family: Tahoma, sans-serif;">Staffan Larsen \
[<a href="mailto:staffan.larsen@oracle.com" style="color: purple; text-decoration: \
underline;">mailto:staffan.larsen@oracle.com</a>]<span \
class="apple-converted-space"> </span><br><b>Sent:</b><span \
class="apple-converted-space"> </span>13 January 2014 12:37<br><b>To:</b><span \
class="apple-converted-space"> </span>Chan, Sunny [Tech]<br><b>Cc:</b><span \
class="apple-converted-space"> </span><a \
href="mailto:serviceability-dev@openjdk.java.net" style="color: purple; \
text-decoration: underline;">serviceability-dev@openjdk.java.net</a><span \
class="Apple-converted-space"> </span><a \
href="mailto:serviceability-dev@openjdk.java.net" style="color: purple; \
text-decoration: underline;">serviceability-dev@openjdk.java.net</a><br><b>Subject:</b><span \
class="apple-converted-space"> </span>Re: [PATCH] 7142035 assert in \
j.l.instrument agents during shutdown when daemon thread is \
running</span><o:p></o:p></div></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"> <o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"> <o:p></o:p></div></div><div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;">On 8 jan 2014, at 16:48, \
Chan, Sunny <<a href="mailto:Sunny.Chan@gs.com" style="color: purple; \
text-decoration: underline;"><span style="color: \
purple;">Sunny.Chan@gs.com</span></a>> wrote:<o:p></o:p></div></div><div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
serif;"><br><br><br><o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-family: \
Helvetica, sans-serif;">In terms of the bug fix itself does it look \
fine?</span><o:p></o:p></div></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;"> <o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;">Yes, it \
does.<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; font-size: \
12pt; font-family: 'Times New Roman', \
serif;"> <o:p></o:p></div></div><div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;">Thanks,<o:p></o:p></div></div><div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', \
serif;">/Staffan<o:p></o:p></div></div></div><div style="margin: 0cm 0cm 0.0001pt; \
font-size: 12pt; font-family: 'Times New Roman', serif;"><span style="font-family: \
Helvetica, sans-serif;"><7142035rev1.patch><o:p></o:p></span></div></div></div><div \
style="margin: 0cm 0cm 0.0001pt; font-size: 12pt; font-family: 'Times New Roman', \
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic