Thanks Stefan, I'm waiting for second reviewer.
Yasumasa 2018年3月28日(水) 18:36 Stefan Johansson <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/ > > > > > > Thanks, > > > > Yasumasa > > > > > > > > 2018-03-28 0:29 GMT+09:00 Stefan Johansson <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/ > >>>> 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/ > >>> > >>> > >>>> 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>: > >>>>>> 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/ > >>>>>> 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 > >>>>>> > >
