Fwd: Re: [PATCH] 6822627: NPE at ReferenceTypeImpl.constantPool
Egor Ushakov
egor.ushakov at jetbrains.com
Fri Sep 2 10:55:11 UTC 2016
The resulting webrev with all fixes:
http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.02/
Thanks,
Egor
On 01.09.2016 20:20, serguei.spitsyn at oracle.com wrote:
> Forwarding to the open serviceability mailing list.
>
> Thanks,
> Serguei
>
> On 9/1/16 10:17, Dmitry Samersoff wrote:
>> Egor,
>>
>> The fix looks good for me.
>>
>> -Dmitry
>>
>>
>> On 2016-09-01 20:01, serguei.spitsyn at oracle.com wrote:
>>> I hope, we got a review from Dmitry.
>>> Dmitry, please, confirm.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 9/1/16 10:00, serguei.spitsyn at oracle.com wrote:
>>>> On 9/1/16 08:41, Egor Ushakov wrote:
>>>>> I've found how to fix this!
>>>>>
>>>>> Add @modules section to the test:
>>>>>
>>>>> ->>> * @modules jdk.jdi/com.sun.tools.jdi
>>>> Nice, thanks!
>>>>
>>>>
>>>>> * @run build TestScaffold VMConnection * @run compile -g
>>>>> ConstantPoolInfoGC.java * @run main ConstantPoolInfoGC
>>>>> Please tell me if you need a new webrev.
>>>> Yes, please.
>>>> Also, please add the copyright comment:
>>>>
>>>>
>>>> /*
>>>> * Copyright (c) 2016, Oracle and/or its affiliates. All rights
>>>> reserved.
>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>> *
>>>> * This code is free software; you can redistribute it and/or
>>>> modify it
>>>> * under the terms of the GNU General Public License version 2
>>>> only, as
>>>> * published by the Free Software Foundation.
>>>> *
>>>> * This code is distributed in the hope that it will be useful, but
>>>> WITHOUT
>>>> * ANY WARRANTY; without even the implied warranty of
>>>> MERCHANTABILITY or
>>>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
>>>> License
>>>> * version 2 for more details (a copy is included in the LICENSE
>>>> file that
>>>> * accompanied this code).
>>>> *
>>>> * You should have received a copy of the GNU General Public License
>>>> version
>>>> * 2 along with this work; if not, write to the Free Software
>>>> Foundation,
>>>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>>> *
>>>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA
>>>> 94065 USA
>>>> * or visit www.oracle.com if you need additional information or
>>>> have any
>>>> * questions.
>>>> */
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>
>>>>
>>>>> Egor
>>>>> On 01.09.2016 17:41, Egor Ushakov wrote:
>>>>>> Serguei, it seems that jdk.jdi module in java 9 does not export
>>>>>> com.sun.tools.jdi package. I have not found a way to get access to
>>>>>> the constantPoolBytesRef field in ReferenceTypeImpl yet. Do you know
>>>>>> if there's a way *in tests* to get the access to unexported
>>>>>> packages? Thanks, Egor
>>>>>> On 01.09.2016 13:35, serguei.spitsyn at oracle.com wrote:
>>>>>>> Hi Egor, Sorry for the long silence. As we discussed before the
>>>>>>> test needs to be in the jtreg form that works for jdk 9. Do you
>>>>>>> have any progress with this? Otherwise, how would we be able to
>>>>>>> ensure the fix works as expected? We could try to push your fix
>>>>>>> without a unit test a couple of month ago. But it is too risky at
>>>>>>> this stage of the development process - sorry. I'm still thinking
>>>>>>> how to help and make your fix in. There might be some other tests
>>>>>>> like this one that requires this kind of special handling. Thanks,
>>>>>>> Serguei On 8/29/16 02:52, Egor Ushakov wrote:
>>>>>>>> Hi guys! Any news on this, is there anything else needed from my
>>>>>>>> side? Thanks! On 03.08.2016 12:24, Egor Ushakov wrote:
>>>>>>>>> Hi Dmitry! I'm not sure what format is expected, the bug is:
>>>>>>>>> http://bugs.java.com/view_bug.do?bug_id=6822627 JDK-6822627 :
>>>>>>>>> java.lang.NullPointerException at
>>>>>>>>> ReferenceTypeImpl.constantPool(ReferenceTypeImpl.java:1025)
>>>>>>>>> webrev: http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.01/
>>>>>>>>> Reviewers were not yet assigned (if Serguei is not one of them).
>>>>>>>>> Egor On 02.08.2016 22:47, Dmitry Samersoff wrote:
>>>>>>>>>> Egor, I'll sponsor the fix. Could you send me commit comments in
>>>>>>>>>> OJDK format NNNNN: Synopsys Summary: Reviewed-by:
>>>>>>>>>> Contributed-by: egor.ushakov at jetbrains.com Webrev URL: -Dmitry
>>>>>>>>>> On 2016-07-25 11:01, Egor Ushakov wrote:
>>>>>>>>>>> Dmitry, could you please sponsor the fix? Thanks, Egor --------
>>>>>>>>>>> Forwarded Message -------- Subject: Re: [PATCH] 6822627:
>>>>>>>>>>> NPE at ReferenceTypeImpl.constantPool Date: Fri, 22 Jul
>>>>>>>>>>> 2016 22:58:59 -0700 From: serguei.spitsyn at oracle.com
>>>>>>>>>>> <serguei.spitsyn at oracle.com> To: Egor Ushakov
>>>>>>>>>>> <egor.ushakov at jetbrains.com> On 7/22/16 03:14, Egor Ushakov
>>>>>>>>>>> wrote:
>>>>>>>>>>>> Serguei, there is no hurry, however if you could recommend
>>>>>>>>>>>> someone who can sponsor the fix, that would be great.
>>>>>>>>>>> Egor, You can ask Dmitry Samersoff. But he is on vacation now.
>>>>>>>>>>> Thanks, Serguei
>>>>>>>>>>>> Thanks anyway, Egor On 22.07.2016 13:07,
>>>>>>>>>>>> serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>> Hi Egor, Unfortunately for this bug I have a 4-week vacation
>>>>>>>>>>>>> starting next Monday. It feels like there is not enough time
>>>>>>>>>>>>> to resolve the testing issue and push the fix. I'd suggest to
>>>>>>>>>>>>> postpone it for a month. Does it work for you? We need to
>>>>>>>>>>>>> find a solution for this test problem. Alternatively, you can
>>>>>>>>>>>>> try to find another sponsor for it. On 7/21/16 01:19, Egor
>>>>>>>>>>>>> Ushakov wrote:
>>>>>>>>>>>>>> Hi Serguei, You probably need jdk, not jre at ${JAVA_HOME}.
>>>>>>>>>>>>>> I do not know how to simulate SoftReference ref being gced
>>>>>>>>>>>>>> without accessing the internal field :(
>>>>>>>>>>>>> I do not have a good idea either.
>>>>>>>>>>>>>> We can try to run jvm with -XX:SoftRefLRUPolicyMSPerMB=1 and
>>>>>>>>>>>>>> invoke gc between the requests, but this does not guarantee
>>>>>>>>>>>>>> SoftReference clearance anyway.
>>>>>>>>>>>>> There only way is to stress by eating a lot of memory.
>>>>>>>>>>>>> However, the tests that use such approach are normally
>>>>>>>>>>>>> unstable. Let's think a little bit more on this. Thanks,
>>>>>>>>>>>>> Serguei
>>>>>>>>>>>>>> Egor On 21.07.2016 8:31, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>>> Hi Egor, How do you run the test? I'm getting the errors:
>>>>>>>>>>>>>>> /var/tmp/sspitsyn/dbg/jdk/test/com/sun/jdi/ConstantPoolInfoGC.java:38:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> error: package com.sun.tools.jdi does not exist import
>>>>>>>>>>>>>>> com.sun.tools.jdi.ReferenceTypeImpl;
>>>>>>>>>>>>>>> ^
>>>>>>>>>>>>>>> /var/tmp/sspitsyn/dbg/jdk/test/com/sun/jdi/ConstantPoolInfoGC.java:74:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> error: cannot find symbol Field
>>>>>>>>>>>>>>> constantPoolBytesRef =
>>>>>>>>>>>>>>> ReferenceTypeImpl.class.getDeclaredField("constantPoolBytesRef");
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ^ symbol:
>>>>>>>>>>>>>>> class ReferenceTypeImpl location: class
>>>>>>>>>>>>>>> ConstantPoolInfoGC 2 errors My test run is:
>>>>>>>>>>>>>>> /java/re/jtreg/4.2/promoted/latest/binaries/jtreg/bin/jtreg
>>>>>>>>>>>>>>> \ -jdk ${JAVA_HOME} \ -J-Dtest.java.opts='-bits d64
>>>>>>>>>>>>>>> -Xmixed -server' \ -Dtest.java.opts='-bits d64 -Xmixed
>>>>>>>>>>>>>>> -server' \
>>>>>>>>>>>>>>> /var/tmp/sspitsyn/dbg/jdk/test/com/sun/jdi/ConstantPoolInfoGC.java
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It is normally enough for any jtreg test run. The test
>>>>>>>>>>>>>>> should not use the internal knowledge of the implementation
>>>>>>>>>>>>>>> as it is in the ConstantPoolInfoGC.java. Thanks, Serguei On
>>>>>>>>>>>>>>> 7/20/16 02:53, Egor Ushakov wrote:
>>>>>>>>>>>>>>>> Yes, all correct, thanks! On 20.07.2016 12:34,
>>>>>>>>>>>>>>>> serguei.spitsyn at 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 at 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 at 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 at 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 at jetbrains.com
>>>>>>>>>>>>>>>>>>>>>>> <mailto:egor.ushakov at 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
>>>>>>>>>>>>>> -- 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
More information about the serviceability-dev
mailing list