[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: PING: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable nu
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2018-04-27 18:30:19
Message-ID: a9334e1e-a458-744d-a7b1-c9a11cc4764f () oracle ! com
[Download RAW message or body]
Hi Yasumasa,
I took at look at it, as I did your initial weberv. Although I didn't
see any issues, this is my first time looking at any of this code. It
would be nice to have someone with more experience with this code than I
have take a look at it, but I don't see that as a requirement as long as
Stefan is confident in his review.
thanks,
Chris
On 4/25/18 5:48 AM, Yasumasa Suenaga wrote:
> PING: Could you review this change?
> I've sent review request about a month ago, but I do not yet get
> second reviewer.
>
>>>>> > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/
>
>
> Yasumasa
>
>
> On 2018/04/10 20:10, Yasumasa Suenaga wrote:
>> PING: Could you review it?
>> We need one more reviewer.
>>
>>>>> > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/
>>
>>
>> Yasumasa
>>
>>
>> On 2018/04/03 21:37, Yasumasa Suenaga wrote:
>>> PING: Could you review it?
>>> This change has been passed Mach5 test.
>>>
>>>>> > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2018/03/28 22:38, Stefan Johansson wrote:
>>>> Mach5 testing looks good.
>>>>
>>>> Can someone in the serviceability team do the second review?
>>>>
>>>> Cheers,
>>>> Stefan
>>>>
>>>> On 2018-03-28 13:32, Yasumasa Suenaga wrote:
>>>>> Thanks Stefan,
>>>>> I'm waiting for second reviewer.
>>>>>
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> 2018年3月28日(水) 18:36 Stefan Johansson <stefan.johansson@oracle.com
>>>>> <mailto:stefan.johansson@oracle.com>>:
>>>>>
>>>>> Hi Yasumasa,
>>>>>
>>>>> Local testing looks good and I've kicked of some additional Mach5
>>>>> testing that will include these tests on all platforms.
>>>>>
>>>>> Cheers,
>>>>> Stefan
>>>>>
>>>>> On 2018-03-28 06:04, Yasumasa Suenaga wrote:
>>>>> > Hi Stefan,
>>>>> >
>>>>> > Thank you for sharing your report!
>>>>> > I could reproduce them on my VM.
>>>>> >
>>>>> > I've fixed them in new webrev, and it works fine on my
>>>>> environment.
>>>>> > Could you check again?
>>>>> >
>>>>> > http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.03/
>>>>> <http://cr.openjdk.java.net/%7Eysuenaga/JDK-8199519/webrev.03/>
>>>>> >
>>>>> >
>>>>> > Thanks,
>>>>> >
>>>>> > Yasumasa
>>>>> >
>>>>> >
>>>>> >
>>>>> > 2018-03-28 0:29 GMT+09:00 Stefan Johansson
>>>>> <stefan.johansson@oracle.com <mailto:stefan.johansson@oracle.com>>:
>>>>> >>
>>>>> >> On 2018-03-27 16:44, Yasumasa Suenaga wrote:
>>>>> >>> Hi Stefan,
>>>>> >>>
>>>>> >>> On 2018/03/27 22:45, Stefan Johansson wrote:
>>>>> >>>> Hi Yasumasa,
>>>>> >>>>
>>>>> >>>> On 2018-03-27 10:56, Yasumasa Suenaga wrote:
>>>>> >>>>> Hi Stefan,
>>>>> >>>>>
>>>>> >>>>> Thank you for your comment.
>>>>> >>>>> I updated webrev:
>>>>> >>>>>
>>>>> >>>>> webrev:
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.01/
>>>>> <http://cr.openjdk.java.net/%7Eysuenaga/JDK-8199519/webrev.01/>
>>>>> >>>> I think the usage of Optional in
>>>>> Expression.setRequired(bool) is a bit
>>>>> >>>> unnecessary. It will create temporary objects and there
>>>>> is no benefit from
>>>>> >>>> just doing two simple if-statements.
>>>>> >>>
>>>>> >>> I fixed it in new webrev:
>>>>> >>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.02/
>>>>> <http://cr.openjdk.java.net/%7Eysuenaga/JDK-8199519/webrev.02/>
>>>>> >>>
>>>>> >>>
>>>>> >>>> I also ran this patch (and the one using forcibly) on my
>>>>> single core VM
>>>>> >>>> and realized that this fix will have to include some
>>>>> awk-file updates to
>>>>> >>>> make the test in test/jdk/sun/tools/jstat pass when
>>>>> Serial in chosen as the
>>>>> >>>> default collector. The tests in
>>>>> test/jdk/sun/tools/jstatd/ are fine.
>>>>> >>>
>>>>> >>> Can you share the failure report?
>>>>> >> It relates to all tests that display the the CGC and the
>>>>> CGCT columns, for
>>>>> >> example in jstatGCOutput1.sh:
>>>>> >> S0C S1C S0U S1U EC EU OC OU
>>>>> MC MU
>>>>> >> CCSC CCSU YGC YGCT FGC FGCT CGC CGCT GCT
>>>>> >> 256.0 256.0 254.0 0.0 2176.0 1025.0 5504.0 920.5
>>>>> 7168.0
>>>>> >> 6839.7 768.0 602.8 2 0.007 0 0.000 - -
>>>>> 0.007
>>>>> >>
>>>>> >> The awk regex needs to be updated to handle '-' for these
>>>>> tests:
>>>>> >> test: sun/tools/jstat/jstatGcCapacityOutput1.sh
>>>>> >> Failed. Execution failed: exit code 1
>>>>> >>
>>>>> >> test: sun/tools/jstat/jstatGcMetaCapacityOutput1.sh
>>>>> >> Failed. Execution failed: exit code 1
>>>>> >>
>>>>> >> test: sun/tools/jstat/jstatGcNewCapacityOutput1.sh
>>>>> >> Failed. Execution failed: exit code 1
>>>>> >>
>>>>> >> test: sun/tools/jstat/jstatGcOldCapacityOutput1.sh
>>>>> >> Failed. Execution failed: exit code 1
>>>>> >>
>>>>> >> test: sun/tools/jstat/jstatGcOldOutput1.sh
>>>>> >> Failed. Execution failed: exit code 1
>>>>> >>
>>>>> >> test: sun/tools/jstat/jstatGcOutput1.sh
>>>>> >> Failed. Execution failed: exit code 1
>>>>> >>
>>>>> >>
>>>>> >>> If it occurs in jstatClassloadOutput1.sh, it relates to
>>>>> JDK-8173942.
>>>>> >>>
>>>>> >>>
>>>>> >>> Thanks,
>>>>> >>>
>>>>> >>> Yasumasa
>>>>> >>>
>>>>> >>>
>>>>> >>>> Thanks,
>>>>> >>>> Stefan
>>>>> >>>>> submit-hs:
>>>>> mach5-one-ysuenaga-JDK-8199519-20180327-0652-16322
>>>>> >>>>>
>>>>> >>>>>
>>>>> >>>>> Thanks,
>>>>> >>>>>
>>>>> >>>>> Yasumasa
>>>>> >>>>>
>>>>> >>>>>
>>>>> >>>>>
>>>>> >>>>> 2018-03-27 0:03 GMT+09:00 Stefan Johansson
>>>>> >>>>> <stefan.johansson@oracle.com
>>>>> <mailto:stefan.johansson@oracle.com>>:
>>>>> >>>>>> Hi Yasumasa,
>>>>> >>>>>>
>>>>> >>>>>> On 2018-03-22 11:35, Yasumasa Suenaga wrote:
>>>>> >>>>>>> Hi all,
>>>>> >>>>>>>
>>>>> >>>>>>> Please review this change:
>>>>> >>>>>>>
>>>>> >>>>>>> JBS:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8199519
>>>>> >>>>>>> webrev:
>>>>> cr.openjdk.java.net/~ysuenaga/JDK-8199519/webrev.00/
>>>>> <http://cr.openjdk.java.net/%7Eysuenaga/JDK-8199519/webrev.00/>
>>>>> >>>>>> The fix seems to make things to work as expected.
>>>>> Manually tested it
>>>>> >>>>>> and
>>>>> >>>>>> Mach5 also looks good.
>>>>> >>>>>>
>>>>> >>>>>> I have some comments regarding the patch. I think
>>>>> 'forcibly' should be
>>>>> >>>>>> rename to something more descriptive. Naming is never
>>>>> easy but I think
>>>>> >>>>>> 'required' would be better, as in, this column is
>>>>> required and not
>>>>> >>>>>> allowed
>>>>> >>>>>> to print '-'. That would also render the code in
>>>>> >>>>>> ExpressionResolver.java to
>>>>> >>>>>> be:
>>>>> >>>>>> return new Literal(isRequired ? 0.0d : Double.NaN);
>>>>> >>>>>> I think that also better explains why we return 0
>>>>> instead of NaN.
>>>>> >>>>>>
>>>>> >>>>>> I would also like to see the forcibly/required state
>>>>> moved into the
>>>>> >>>>>> Expression it self, that way we don't have to pass it
>>>>> around but can
>>>>> >>>>>> instead
>>>>> >>>>>> do:
>>>>> >>>>>> return new Literal(e.isRequired() ? 0.0d :
>>>>> Double.NaN);
>>>>> >>>>>>
>>>>> >>>>>> Thanks,
>>>>> >>>>>> Stefan
>>>>> >>>>>>
>>>>> >>>>>>
>>>>> >>>>>>> After JDK-8153333, some jstat tests are failed because
>>>>> GCT in jstat
>>>>> >>>>>>> output
>>>>> >>>>>>> is dash (-) if garbage collector is not concurrent
>>>>> collector e.g.
>>>>> >>>>>>> Serial GC.
>>>>> >>>>>>> I fixed that GCT can be calculated correctly.
>>>>> >>>>>>>
>>>>> >>>>>>> This change has been tested on Mach5 by Stefan.
>>>>> >>>>>>>
>>>>> >>>>>>>
>>>>> >>>>>>> Thanks,
>>>>> >>>>>>>
>>>>> >>>>>>> Yasumasa
>>>>> >>>>>>
>>>>>
>>>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic