[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: Fwd: Re: [PATCH] 6822627: NPE at ReferenceTypeImpl.constantPool
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2016-09-07 1:10:34
Message-ID: 7b8f7fae-912e-45f2-be0e-f3d9a923cdb5 () oracle ! com
[Download RAW message or body]
On 9/4/16 20:12, serguei.spitsyn@oracle.com wrote:
> Hi Egor,
>
> Unfortunately, we are in the RDP1 phase from September 1-st.
> Only P1-P3 bugs are allowed for fix.
> We need to find out what is the process if we want to fix this bug in
> RDP1.
> I'll check with colleagues what options exist.
I've set the priority to P3 so that it is Ok to push the fix.
Thanks,
Serguei
>
> Thanks,
> Serguei
>
>
> On 9/2/16 03:55, Egor Ushakov wrote:
>> 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@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@oracle.com wrote:
>>>>> I hope, we got a review from Dmitry.
>>>>> Dmitry, please, confirm.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 9/1/16 10:00, serguei.spitsyn@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@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@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@oracle.com
>>>>>>>>>>>>> <serguei.spitsyn@oracle.com> To: Egor Ushakov
>>>>>>>>>>>>> <egor.ushakov@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@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@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@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
>>>>>>>>>>>>>>>> -- 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
>>>>
>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic