PING: RFR: 8199519: Several GC tests fails with: java.lang.NumberFormatException: Unparseable number: "-"
David Holmes
david.holmes at oracle.com
Tue May 1 07:35:00 UTC 2018
Hi Yasumasa,
On 1/05/2018 2:07 PM, Yasumasa Suenaga wrote:
> Hi David,
>
> On 2018/05/01 10:36, David Holmes wrote:
>> Hi Yasumasa,
>>
>> This appears to have been pushed without the second review occurring.
>> Chris's email appears to be from after when this was pushed.
>
> I got two reviewers.
> According to [1], we need to get 1 reviewer and 1 committer or reviewer
> before pushing. I got them. [2] [3]
My apologies - I did not have Jini's email for some reason. I only saw
your request for a second review then Chris's later reply.
>
>> This change also seems to be causing, or contributing to a failure in
>> serviceability/tmtools/jstat/GcTest01.java:
>>
>> java.lang.RuntimeException: Number of concurrent GC events is 1, but
>> CGCT is 0
>>
>> I will file a bug for that and assign to you for evaluation.
>
> Okay, I will try it, but I do not have OS X.
> Can you share .jtr file? Please attach it to JBS if you can.
I can add key part to a comment.
Thanks,
David
>
> Thanks,
>
> Yasumasa
>
>
> [1]
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-March/030656.html
> [2]
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-March/023419.html
>
> [3]
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-April/023590.html
>
>
>
>> Thanks,
>> David
>>
>> On 25/04/2018 10:48 PM, 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 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