Re: RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-07 Thread Alan Bateman 

On 08/03/2018 07:27, Amit Sapre wrote:


Hello,

Please review the changes for removing SNMP support.

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8071367

Webrev : 
http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.00 



cc'ing compiler-dev for help on javac/resources/legacy.properties. I'm 
not 100% sure if it is used when compiling to old releases or not.


As you are re-wording the class description for jdk.internal.agent.Agent 
then we might as well get it right. The Agent class loaded and its 
static no-arg startAgent method is invoked when a system property 
starting with "com.sun.management" is specified on the command line. We 
could expand this to include the case where it is started in a running 
VM too.


build.properties - I assume the empty value for excludes shouldn't have 
a continuation character now.


The rest looks good to me.

-Alan.

RFR : JDK-8071367 - JMX: Remove SNMP support

2018-03-07 Thread Amit Sapre 
Hello,

 

Please review the changes for removing SNMP support.

 

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8071367 

Webrev : http://cr.openjdk.java.net/~asapre/webrev/2018/JDK-8071367/webrev.00

 

Thanks,

Amit

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

2018-03-07 Thread Yasumasa Suenaga 

PING: Could you review it?

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

JBS: https://bugs.openjdk.java.net/browse/JDK-815
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-815/webrev.07/


JBS: https://bugs.openjdk.java.net/browse/JDK-815
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-815/webrev.07/


Thanks,

Yasumasa


[1] https://bugs.openjdk.java.net/browse/JDK-8196862



2018-02-06 22:33 GMT+09:00 Yasumasa Suenaga :

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-815/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-815-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 :



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-815/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-815/webrev.04/



This change has been passed Mach 5 via submit repo: