PING: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable number: "-"
Yasumasa Suenaga
yasuenag at gmail.com
Wed Apr 25 12:48:59 UTC 2018
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 at oracle.com <mailto:stefan.johansson at 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 at oracle.com <mailto:stefan.johansson at 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 at oracle.com <mailto:stefan.johansson at 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
>>>> >>>>>>
>>>>
>>>
More information about the serviceability-dev
mailing list