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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: PING: RFR: JDK-8153333: [REDO] STW phases at Concurrent GC should count in PerfCounter
From:       Yasumasa Suenaga <yasuenag () gmail ! com>
Date:       2018-02-21 12:14:31
Message-ID: 9183dfd3-522f-fd73-a681-5ea957f1d717 () gmail ! com
[Download RAW message or body]

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@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@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
>>>>>>>>>>>>>
>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic