PING: RFR: JDK-8153333: [REDO] STW phases at Concurrent GC should count in PerfCounter

Wed Mar 7 12:18:41 UTC 2018

PING: Could you review it?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.08/

JBS: https://bugs.openjdk.java.net/browse/JDK-8153333
CSR: https://bugs.openjdk.java.net/browse/JDK-8196862

This change has passed Mach5 on submit repo.
Also it has passed hotspot/jtreg/:hotspot_serviceability and jdk/:jdk_tools jtreg tests.

We need one more reviewer.

Thanks,

Yasumasa

On 2018/02/21 21:14, Yasumasa Suenaga wrote:
> PING: Could you review it?
> 
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.07/
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8153333
> CSR: https://bugs.openjdk.java.net/browse/JDK-8196862
> 
> 
> Yasumasa
> 
> 
> On 2018/02/15 10:23, Yasumasa Suenaga wrote:
>> Hi all,
>>
>> CSR for this issue [1] has been approved.
>> This webrev has been reviewed by Stefan, but we need one more
>> reviewer. Could you review it?
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.07/
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8196862
>>
>>
>>
>> 2018-02-06 22:33 GMT+09:00 Yasumasa Suenaga <yasuenag at gmail.com>:
>>> Hi Stefan,
>>>
>>>> This looks good to me, will do some more testing while waiting for a
>>>> second reviewer and I can sponsor the change once it's ready to go.
>>>
>>>
>>> Thanks! I'm waiting for second reviewer.
>>>
>>>>> What should I do to get CSR approve?
>>>>
>>>> In the bug system under "More" you can choose "Create CSR" which is the
>>>> first step. More information can be found on the wiki:
>>>> https://wiki.openjdk.java.net/display/csr/CSR+FAQs
>>>
>>>
>>> I filed new CSR:
>>>    https://bugs.openjdk.java.net/browse/JDK-8196862
>>>
>>>
>>> Yasumasa
>>>
>>>
>>>
>>> On 2018/02/06 21:55, Stefan Johansson wrote:
>>>>
>>>>
>>>>
>>>> On 2018-02-06 06:10, Yasumasa Suenaga wrote:
>>>>>
>>>>> Hi Stefan,
>>>>>
>>>>>> I agree, for G1 this should not be controlled. Maybe I was a bit
>>>>>> unclear, I
>>>>>> was wondering why we want to control it for CMS.
>>>>>
>>>>> I said to remove -XX:EnableConcGCPerfCounter in two years ago. I've
>>>>> missed it :-)
>>>>>
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/017125.html
>>>>>
>>>>> So I uploaded new webrev. This change includes copyright year updates.
>>>>>
>>>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.06/
>>>>
>>>> Thanks Yasumasa,
>>>>
>>>> This looks good to me, will do some more testing while waiting for a
>>>> second reviewer and I can sponsor the change once it's ready to go.
>>>>>
>>>>>
>>>>> This change passes all tests on submit repo, and
>>>>> :hotspot_serviceability :jdk_tools tests on my laptop.
>>>>>
>>>>>
>>>>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8153333-20180206-0222-10428
>>>>>
>>>>>
>>>>>> If we do the change for CMS, we should
>>>>>> probably also do a CSR, but that should be fairly straight forward.
>>>>>
>>>>> What should I do to get CSR approve?
>>>>
>>>> In the bug system under "More" you can choose "Create CSR" which is the
>>>> first step. More information can be found on the wiki:
>>>> https://wiki.openjdk.java.net/display/csr/CSR+FAQs
>>>>
>>>> Cheers,
>>>> Stefan
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> 2018-02-06 0:33 GMT+09:00 Stefan Johansson <stefan.johansson at oracle.com>:
>>>>>>
>>>>>>
>>>>>> On 2018-02-03 06:40, Yasumasa Suenaga wrote:
>>>>>>>
>>>>>>> On 2018/02/02 23:38, Stefan Johansson wrote:
>>>>>>>>
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> The changes doesn't apply clean on the latest jdk/hs, can you provide
>>>>>>>> an
>>>>>>>> updated webrev?
>>>>>>>
>>>>>>>
>>>>>>> I uploaded webrev for jdk-hs:
>>>>>>>     cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.05/
>>>>>>>
>>>>>> Thanks, I've kicked off a testing job now to verify nothing unexpected
>>>>>> fails.
>>>>>>>
>>>>>>>
>>>>>>>> The testing done by the submit repo doesn't cover the tests you have
>>>>>>>> update so I plan to take the change for a spin and make sure the
>>>>>>>> correct
>>>>>>>> tests are run and verified in Mach 5.
>>>>>>>
>>>>>>>
>>>>>>> I've also tested hotspot/jtreg/:hotspot_serviceability and
>>>>>>> jdk/:jdk_tools
>>>>>>> on my laptop.
>>>>>>> I did not see any errors / failures which are related to this change.
>>>>>>
>>>>>> I also ran some local tests on this and it looks good.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Also a question about the change. Why do we need a special flag for
>>>>>>>> CMS?
>>>>>>>> I see that the original bug report refers to the flag as being a way
>>>>>>>> to turn
>>>>>>>> on and off the feature but the current implementation only consider
>>>>>>>> the flag
>>>>>>>> for CMS.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/016774.html
>>>>>>>
>>>>>>> Originally, STW phases (Remark and Cleanup) at G1 are not counted in
>>>>>>> jstat
>>>>>>> FGC column.
>>>>>>> So I think we need not to control the behavior of PerfCounter for G1.
>>>>>>>
>>>>>> I agree, for G1 this should not be controlled. Maybe I was a bit
>>>>>> unclear, I
>>>>>> was wondering why we want to control it for CMS. I think either we
>>>>>> should
>>>>>> change the behavior without guarding it by a flag or just skip updating
>>>>>> CMS
>>>>>> (and leave the pauses in FGC). If we do the change for CMS, we should
>>>>>> probably also do a CSR, but that should be fairly straight forward.
>>>>>>
>>>>>> I also found the old review thread where Jon M had the same comment
>>>>>> (removing the flag) and it looks like all agreed on that:
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-March/017118.html
>>>>>>
>>>>>> Thanks,
>>>>>> Stefan
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Stefan
>>>>>>>>
>>>>>>>> On 2018-02-01 14:58, Yasumasa Suenaga wrote:
>>>>>>>>>
>>>>>>>>> PING: Could you review and sponsor it?
>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.04/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This change has been passed Mach 5 via submit repo:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://java.se.oracle.com:10065/mdash/jobs/mach5-one-ysuenaga-JDK-8153333-20180201-0805-10101
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2017/11/01 22:02, Yasumasa Suenaga wrote:
>>>>>>>>>>
>>>>>>>>>> PING: Could you review and sponsor it?
>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.04/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Also I need JPRT results of this change.
>>>>>>>>>> Could you cooperate?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2017/09/27 0:08, Yasumasa Suenaga wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> I uploaded new webrev to be adapted to jdk10/hs:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.04/
>>>>>>>>>>>
>>>>>>>>>>> I want to check this patch via JPRT, but I cannot access it.
>>>>>>>>>>> Could you cooperate?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> yasumasa
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2017/09/21 7:46, Yasumasa Suenaga wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> PING:
>>>>>>>>>>>>
>>>>>>>>>>>> Have you checked this issue?
>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.03/hotspot/
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.03/jdk/
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2017/07/01 23:44, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> PING:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Have you checked this issue?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2017/06/14 13:22, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I changed PerfCounter to show CGC STW phase in jstat in
>>>>>>>>>>>>>> JDK-8151674.
>>>>>>>>>>>>>> However, it occurred several jtreg test failure, so it was
>>>>>>>>>>>>>> back-outed.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I want to resume to work for this issue.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.03/hotspot/
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8153333/webrev.03/jdk/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> These changes are work fine on jtreg test as below:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>      hotspot/test/serviceability/tmtools/jstat
>>>>>>>>>>>>>>      jdk/test/sun/tools
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Since JDK 9, default GC algorithm is set to G1.
>>>>>>>>>>>>>> So I think this change is useful to watch GC behavior through
>>>>>>>>>>>>>> jstat.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I cannot access JPRT. Could you help?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>
>>>>
>>>