Thanks Stefan,
I'm waiting for second reviewer.
Yasumasa
2018年3月28日(水) 18:36 Stefan Johansson
<stefan.johans...@oracle.com <mailto:stefan.johans...@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.johans...@oracle.com <mailto:stefan.johans...@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.johans...@oracle.com
<mailto:stefan.johans...@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
>>>>>>
