[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: [PATCH] 6822627: NPE at ReferenceTypeImpl.constantPool
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2016-07-20 9:56:10
Message-ID: 578F4ABA.6010606 () oracle ! com
[Download RAW message or body]
On 7/20/16 02:53, Egor Ushakov wrote:
>
> Yes, all correct, thanks!
>
Ok, thanks.
Serguei
>
> On 20.07.2016 12:34, serguei.spitsyn@oracle.com wrote:
> > Egor,
> >
> > If I understand correctly, you do not have an openJdk user ID and an
> > author status.
> > Please, see the link:
> > http://openjdk.java.net/census
> >
> > So that, I'll commit your fix with the comment:
> > Contributed-by: egor.ushakov@jetbrains.com
> >
> > and push it to the jdk9/hs.
> >
> > I hope, somebody will correct me if it is not right.
> > Please, let me know if it works for you.
> >
> > Thanks,
> > Serguei
> >
> >
> > On 7/20/16 02:10, Egor Ushakov wrote:
> > >
> > > Serguei, thanks for the review!
> > >
> > > Please sponsor the fix, I do not know how to proceed.
> > >
> > > Thanks!
> > >
> > > Egor
> > >
> > >
> > > On 20.07.2016 10:36, serguei.spitsyn@oracle.com wrote:
> > > > Hi Egor,
> > > >
> > > > Thank you for providing the test!
> > > >
> > > > A couple of nits to the test:
> > > >
> > > > 56 if (!Arrays.equals(cpbytes, cpbytes2)) {
> > > > 57 failure("Consequent constantPool results vary, first was : \
> > > > " + cpbytes + ", now: " + cpbytes2); 58 };
> > > > Last semicolon is not need.
> > > > 74 if (!testFailed) {
> > > > 75 println("ConstantPoolInfoGC: passed");
> > > > 76 } else {
> > > > 77 throw new Exception("ConstantPoolInfoGC: failed");
> > > > 78 }
> > > > I'd suggest to rearrange the statement above to something like this:
> > > > 74 if (testFailed) {
> > > > 75 throw new Exception("ConstantPoolInfoGC: failed");
> > > > 76 }
> > > > 77 println("ConstantPoolInfoGC: passed");
> > > >
> > > > But I leave it up to you. No need to see another webrev. I
> > > > can sponsor your fix, if needed. Thanks, Serguei On 7/19/16 00:07,
> > > > Egor Ushakov wrote:
> > > > >
> > > > > Hi Serguei,
> > > > >
> > > > > here's a new webrev with a test included:
> > > > > http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.01/
> > > > >
> > > > > Egor
> > > > >
> > > > > On 15.07.2016 12:22, serguei.spitsyn@oracle.com wrote:
> > > > > > Hi Egor, The fix looks good modulo the synchronization issue. Do
> > > > > > you have a jtreg unit test reproducing this failure mode? We have
> > > > > > a rule to provide a test coverage for the fixes. Thanks, Serguei
> > > > > > On 7/15/16 00:03, Egor Ushakov wrote:
> > > > > > >
> > > > > > > Thanks for your comments!
> > > > > > >
> > > > > > > I totally agree that the code there requires major refactoring.
> > > > > > > In the fix I tried not to make it worse and fix the NPE.
> > > > > > >
> > > > > > > On 14.07.2016 20:06, Martin Buchholz wrote:
> > > > > > > > The lack of volatile or synchronization, plus the use of
> > > > > > > > multiple mutable fields, suggests that a deeper fix is
> > > > > > > > necessary to be truly correct. For comparison, much of the
> > > > > > > > similar code implementing reflection has been changed to use
> > > > > > > > volatile. But that's a big job, for the serviceability team.
> > > > > > > > On Thu, Jul 14, 2016 at 2:33 AM, Egor Ushakov
> > > > > > > > <egor.ushakov@jetbrains.com
> > > > > > > > <mailto:egor.ushakov@jetbrains.com>> wrote:
> > > > > > > >
> > > > > > > > Hi, I'm a developer from IDEA debugger (we have company
> > > > > > > > OCA). We've increased usage of
> > > > > > > > ReferenceTypeImpl.constantPool and now facing JDK-6822627
> > > > > > > > more often. Please review and sponsor the fix. The problem
> > > > > > > > was that constantPoolBytesRef SoftReference value coud be
> > > > > > > > GCed and there was no check for that. I've added it to the
> > > > > > > > check inside getConstantPoolInfo to request the bytes again
> > > > > > > > in this case. BUGURL:
> > > > > > > > https://bugs.openjdk.java.net/browse/JDK-6822627 WEBREV:
> > > > > > > > http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.00/
> > > > > > > > <http://cr.openjdk.java.net/%7Eavu/JDK-6822627/webrev.00/>
> > > > > > > > Thanks!-- Egor Ushakov Software Developer JetBrains
> > > > > > > > http://www.jetbrains.com The Drive to Develop
> > > > > > > >
> > > > > > > --
> > > > > > > Egor Ushakov
> > > > > > > Software Developer
> > > > > > > JetBrains
> > > > > > > http://www.jetbrains.com
> > > > > > > The Drive to Develop
> > > > > --
> > > > > Egor Ushakov
> > > > > Software Developer
> > > > > JetBrains
> > > > > http://www.jetbrains.com
> > > > > The Drive to Develop
> > > --
> > > Egor Ushakov
> > > Software Developer
> > > JetBrains
> > > http://www.jetbrains.com
> > > The Drive to Develop
> --
> Egor Ushakov
> Software Developer
> JetBrains
> http://www.jetbrains.com
> The Drive to Develop
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 7/20/16 02:53, Egor Ushakov wrote:<br>
</div>
<blockquote
cite="mid:342df83e-e7fe-4f8b-eaa6-6aa1427f6b45@jetbrains.com"
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<p>Yes, all correct, thanks!<br>
</p>
</blockquote>
Ok, thanks.<br>
Serguei<br>
<br>
<br>
<blockquote
cite="mid:342df83e-e7fe-4f8b-eaa6-6aa1427f6b45@jetbrains.com"
type="cite">
<p> </p>
<br>
<div class="moz-cite-prefix">On 20.07.2016 12:34, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"><a \
class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a></a> \
wrote:<br> </div>
<blockquote cite="mid:578F459B.8070008@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Egor,<br>
<br>
If I understand correctly, you do not have an openJdk user ID
and an author status.<br>
Please, see the link:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://openjdk.java.net/census">http://openjdk.java.net/census</a><br>
<br>
So that, I'll commit your fix with the comment:<br>
Contributed-by: <a moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:egor.ushakov@jetbrains.com">egor.ushakov@jetbrains.com</a><br>
<br>
and push it to the jdk9/hs.<br>
<br>
I hope, somebody will correct me if it is not right.<br>
Please, let me know if it works for you.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 7/20/16 02:10, Egor Ushakov wrote:<br>
</div>
<blockquote
cite="mid:04eb5e4d-eccb-d168-4701-6e8badcbd43a@jetbrains.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<p>Serguei, thanks for the review!</p>
<p>Please sponsor the fix, I do not know how to proceed.</p>
<p>Thanks!</p>
<p>Egor<br>
</p>
<br>
<div class="moz-cite-prefix">On 20.07.2016 10:36, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"><a \
class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a></a> \
wrote:<br> </div>
<blockquote cite="mid:578F2A0B.4070806@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Egor,<br>
<br>
Thank you for providing the test!<br>
<br>
A couple of nits to the test:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<pre><meta http-equiv="content-type" content="text/html; \
charset=utf-8"> 56 if (!Arrays.equals(cpbytes, cpbytes2)) { 57 \
failure("Consequent constantPool results vary, first was : " + cpbytes + ", now: " + \
cpbytes2); 58 };</pre> Last semicolon is not need.
<meta http-equiv="content-type" content="text/html; charset=utf-8"><pre> 74 \
if (!testFailed) { 75 println("ConstantPoolInfoGC: passed");
76 } else {
77 throw new Exception("ConstantPoolInfoGC: failed");
78 }</pre>
I'd suggest to rearrange the statement above to something like this:
<meta http-equiv="content-type" content="text/html; charset=utf-8"><pre> 74 \
if (testFailed) { 75 throw new Exception("ConstantPoolInfoGC: failed");
76 }
77 println("ConstantPoolInfoGC: passed");
</pre> But I leave it up to you.
No need to see another webrev.
I can sponsor your fix, if needed.
Thanks,
Serguei
On 7/19/16 00:07, Egor Ushakov wrote:
</div><blockquote cite="mid:f1c9d6a0-4bfb-a8e6-5f9d-97e42e8f61c6@jetbrains.com" \
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<p>Hi Serguei,</p>
<p>here's a new webrev with a test included:
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Eavu/JDK-6822627/webrev.01/">http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.01/</a></p>
<p>Egor
</p>
<div class="moz-cite-prefix">On 15.07.2016 12:22,
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:
</div>
<blockquote cite="mid:5788AB52.40605@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Egor,
The fix looks good modulo the synchronization issue.
Do you have a jtreg unit test reproducing this failure mode?
We have a rule to provide a test coverage for the fixes.
Thanks,
Serguei
On 7/15/16 00:03, Egor Ushakov wrote:
</div>
<blockquote cite="mid:b964cc5e-826f-cd93-8090-46b2db6a0582@jetbrains.com" \
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<p>Thanks for your comments!</p>
<p>I totally agree that the code there requires major
refactoring. In the fix I tried not to make it worse and fix
the NPE.
</p>
<div class="moz-cite-prefix">On 14.07.2016 20:06, Martin
Buchholz wrote:
</div>
<blockquote cite="mid:CA+kOe083Wv3=L0pNJUZTToS2DB+1BvE+-B7rYEhhjBECQm6axQ@mail.gmail.com" \
type="cite"> <div dir="ltr">The lack of volatile or synchronization, plus
the use of multiple mutable fields, suggests that a deeper
fix is necessary to be truly correct. For comparison, much
of the similar code implementing reflection has been changed
to use volatile. But that's a big job, for the
serviceability team.</div>
<div class="gmail_extra">
<div class="gmail_quote">On Thu, Jul 14, 2016 at 2:33 AM,
Egor Ushakov <span dir="ltr"><<a moz-do-not-send="true" \
href="mailto:egor.ushakov@jetbrains.com" \
target="_blank">egor.ushakov@jetbrains.com</a>></span> wrote:
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,
I'm a developer from IDEA debugger (we have company
OCA).
We've increased usage of ReferenceTypeImpl.constantPool
and now facing
JDK-6822627 more often.
Please review and sponsor the fix.
The problem was that constantPoolBytesRef SoftReference
value coud be
GCed and there was no check for that.
I've added it to the check inside getConstantPoolInfo to
request the
bytes again in this case.
BUGURL: <a moz-do-not-send="true" \
href="https://bugs.openjdk.java.net/browse/JDK-6822627" rel="noreferrer" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-6822627</a>
WEBREV: <a moz-do-not-send="true" \
href="http://cr.openjdk.java.net/%7Eavu/JDK-6822627/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.00/</a>
Thanks!<span class="HOEnZb"><font color="#888888">
--
Egor Ushakov
Software Developer
JetBrains
<a moz-do-not-send="true" href="http://www.jetbrains.com" \
rel="noreferrer" target="_blank">http://www.jetbrains.com</a>
The Drive to Develop
</font></span></blockquote>
</div>
</div>
</blockquote>
<pre class="moz-signature" cols="72">--
Egor Ushakov
Software Developer
JetBrains
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://www.jetbrains.com">http://www.jetbrains.com</a> The Drive to \
Develop</pre> </blockquote>
</blockquote>
<pre class="moz-signature" cols="72">--
Egor Ushakov
Software Developer
JetBrains
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://www.jetbrains.com">http://www.jetbrains.com</a> The Drive to \
Develop</pre>
</blockquote>
</blockquote>
<pre class="moz-signature" cols="72">--
Egor Ushakov
Software Developer
JetBrains
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://www.jetbrains.com">http://www.jetbrains.com</a> The Drive to \
Develop</pre>
</blockquote>
</blockquote>
<pre class="moz-signature" cols="72">--
Egor Ushakov
Software Developer
JetBrains
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://www.jetbrains.com">http://www.jetbrains.com</a> The Drive to \
Develop</pre>
</blockquote>
</body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic