Fwd: Re: [PATCH] 6822627: NPE at ReferenceTypeImpl.constantPool
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Sep 1 17:20:00 UTC 2016
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
>
More information about the serviceability-dev
mailing list