Hi Lin,

The latest webrev looks good to me.
Just want to double check, how did you check no regressions are introduced with your fix?

Thanks,
Serguei



On 8/11/20 08:22, linzang(臧琳) wrote:

Hi Serguei,

                Thanks a lot for your advice. I agree your concern and will take care of it in future.  

                Here is the latest webrev based on your comments:  (delta is just retrieving the usage(1))

                http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev14/

              So may I assume that the patch is OK with you now?

Hi All,

                In summary, Here are the status of this change at present:

                * Paul and Serguei have helped review the runtime/JMap part and the changes now is Okay with them.

                * Stefan has helped review the GC part and it is Okay with him now.

                So does it need more review and approval for pushing this change?

 

BRs,

Lin

 

On 2020/8/11, 2:40 PM, "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> wrote:

 

    Hi Lin,

 

    I prefer a conservative approach and do not change things without a real

    need.

 

    Thanks,

    Serguei

 

    On 8/10/20 20:23, linzang(臧琳) wrote:

    > Hi Serguei

    >      I got your point, just thought usage may be a little verbosity, it prints almost my whole screen which could flush the error message. And I checked that other jcmd tools usually use System.exit() after print errors. So I made the change.

    >

   > Thanks!

    > Lin

    >

    >> On Aug 11, 2020, at 11:05 AM, "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> wrote:

    >>

    >> Hi Lin,

    >>

    >> I've re-reviewed the JMap.java only.

    >> It looks good except there was no need to replace the usage(1) call with the System.exit(1).

    >> I did not say usage is not needed, just that it is not enough.

    >>

    >> Thanks,

    >> Serguei

    >>

    >>

    >>> On 8/10/20 19:25, linzang(臧琳) wrote:

    >>> Hi Serguei,

    >>>           >> First, the CSR does not include any update for 'live' and 'all' options, does it?

    >>>     >> If so, then I'm confused why do you need all these changes related to these two options.

    >>>     >> Did you intend to really change anything?

    >>>      Yes, you’re correct, CSR doesn’t mention any thing about “live” and “all”. so all those changes related become unnecessary.

    >>>

    >>>      Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_13

    >>>      Delta: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_13_delta

    >>>

    >>>      BTW, during refining this changeset I also found an issue that jmap -dump could accept undefined options, will setup a new issue in JBS and fix it separately soon.

    >>>

    >>>

    >>> BRs,

    >>> Lin

    >>>

    >>> From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>

    >>> Date: Tuesday, August 11, 2020 at 8:40 AM

    >>> To: "linzang(臧琳)" <linz...@tencent.com>, "Hohensee, Paul" <hohen...@amazon.com>, Stefan Karlsson <stefan.karls...@oracle.com>, David Holmes <david.hol...@oracle.com>, serviceability-dev <serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net" <hotspot-gc-...@openjdk.java.net>

    >>> Subject: Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)

    >>>

    >>> Hi Lin,

    >>>

    >>> A couple of things.

    >>>

    >>> First, the CSR does not include any update for 'live' and 'all' options, does it?

    >>> If so, then I'm confused why do you need all these changes related to these two options.

    >>> Did you intend to really change anything?

    >>>

    >>> Second, new error messages do not look useful as they say nothing about what is wrong.

    >>> Printing usage does not help either.

    >>> Could these messages be more specific?

    >>> My suggestions are:

    >>>   188                 if (filename == null) {

    >>>   189                     System.err.println("Fail at processing option '" + subopt +"'");

    >>>   190                     usage(1); // invalid options or no filename

    >>>   191                 }

    >>>    System.err.println("Fail: invalid option or no file name: '" + subopt +"'");

    >>>   194                if (parallel == null) {

    >>>   195                     System.err.println("Fail at processing option '" + subopt + "'");

    >>>   196                     usage(1);

    >>>   197                }

    >>>    System.err.println("Fail: no number provided in option: '" + subopt +"'");

    >>>   198             } else {

    >>>   199                 System.err.println("Fail at processing option '" + subopt + "'");

    >>>   200                 usage(1);

    >>>   201             }

    >>>    System.err.println("Fail: invalid option: '" + subopt +"'");

    >>>

    >>>

    >>> The default value is listed in the 'parallel' flag description:

    >>>    parallel=<count> generate histogram using this many parallel threads, default 0

    >>> It means that the flag is optionl.

    >>>

    >>> I'm okay to file a separate enhancement to add a clarification for 'live' and 'all' flags.

    >>>

    >>> Thanks,

    >>> Serguei

    >>>

    >>>

    >>> On 8/10/20 16:46, linzang(臧琳) wrote:

    >>> And Here is the latest refined changeset

    >>> Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_12/

    >>> Delta: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_12_delta/

    >>>

    >>> BRs,

    >>> Lin

    >>>

    >>> On 2020/8/11, 7:23 AM, "linzang(臧琳)" mailto:linz...@tencent.com wrote:

    >>>

    >>>      Dear Serguei,

    >>>                Here is my reply for your question about non-numeric value for “parallel” (somehow the thread of replay became out of order, not sure why).

    >>>

    >>>      >      >> What is going to happen if the resulting 'parallel' substring above is not a number?

    >>>      >      The error handling logic locates at http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11/src/hotspot/share/services/attachListener.cpp.frames.html, (line 276-284)

    >>>      >      Generally, the result is error message will be print if “parallel” is illegal.  An example output would be:

    >>>      >     ############################

   >>>      >          $ time jmap -histo:parallel=c 26233

    >>>      >        Exception in thread "main" com.sun.tools.attach.AttachOperationFailedException: Invalid parallel thread number: [c]

    >>>      >                                              at jdk.attach/sun.tools.attach.VirtualMachineImpl.execute(VirtualMachineImpl.java:227)

    >>>      >                                               at jdk.attach/sun.tools.attach.HotSpotVirtualMachine.executeCommand(HotSpotVirtualMachine.java:309)

    >>>      >                                               at jdk.jcmd/sun.tools.jmap.JMap.executeCommandForPid(JMap.java:133)

    >>>      >                                                at jdk.jcmd/sun.tools.jmap.JMap.histo(JMap.java:206)

    >>>      >                                               at jdk.jcmd/sun.tools.jmap.JMap.main(JMap.java:112)

    >>>      >

    >>>      >    ############################

    >>>      >

    >>>      > Hi Serguei, Paul and Stefan.

    >>>      >      Moreover, I will made a new changeset with following changes:

    >>>      >    * Print error message + usage when parameter check fail in Jmap.java

    >>>      >    *Retrive the histo logic that if “all” and “live” are set at same time, use “live”, rather than print error message. (not sure which one is better :P)

    >>>

    >>>      My last point is to retrive the behavior for compatibility.  And do you think make a separate enhancement about spec is reasonable ?

    >>>

    >>>      Thanks!

    >>>

    >>>      BRs,

    >>>      Lin

    >>>

    >>>      From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com

    >>>      Date: Tuesday, August 11, 2020 at 5:11 AM

    >>>      To: "linzang(臧琳)" mailto:linz...@tencent.com, "Hohensee, Paul" mailto:hohen...@amazon.com, Stefan Karlsson mailto:stefan.karls...@oracle.com, David Holmes mailto:david.hol...@oracle.com, serviceability-dev mailto:serviceability-dev@openjdk.java.net, mailto:hotspot-gc-...@openjdk.java.net mailto:hotspot-gc-...@openjdk.java.net

    >>>      Subject: Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)

    >>>

    >>>      Hi Lin,

    >>>

    >>>

    >>>      On 8/7/20 03:41, linzang(臧琳) wrote:

    >>>      Dear Serguei,

    >>>               Thanks a lot for your review!

    >>>      >> The spec says nothing if the new option 'parallel' is mandatory or optional.

    >>>      >> Also, (it was before your fix) the spec does not say if the options 'live' and 'all' are mutually exclusive.

    >>>               For “parallel”,  the spec adds “parallel=0” is the default behavior. So my assumption is if parallel is not used, it will be 0.  Do you think it is ok ? is it necessary to obviously add comments like “if no parallel is set, use the default value 0”?

    >>>

    >>>      It'd be nice to make it clear.

    >>>      But the CSR will need to be updated.

    >>>      In fact, I did not want you to go through this cycle again.

    >>>      But maybe it is worth to improve the specs in this regard.

    >>>      May be Paul has some alternative suggestions.

    >>>

    >>>

    >>>               For “live” and “all”, before the changeset , I see the logic from the code is that both of them can be set at the same time, and the “live” will take effect. IMHO this may be a little confused. So I made the change, not sure whether I should keep the same behavior as before in this change?

    >>>

    >>>      This is better to clearly specify what is allowed and what is the behavior.

    >>>

    >>>

    >>>               And I like your idea of printing more error msg if something wrong with the options setting, but I checked that before the change, if there is not a match option, it only print usage. and not only jmap -histo but also jmap -dump has this issue, do you agree if I fix both in the changeset?

    >>>

    >>>      Yes, it'd be nice to make it clear in both specs.

    >>>

    >>>                      >> What is going to happen if null is passed in place of parallel here? :

    >>>              The default value 0 will be used if no “parallel” option is set.

    >>>

    >>>      Okay, thanks.

    >>>

    >>>

    >>>                                     >>  Should the lines 193-195 be moved after the line 202?

    >>>               I don’t think so, the logic is a little different.  At line 193, the case is “parallel=<blank>”.   If move them to line 203, it mean “parallel” is not optional.

    >>>      Okay, I see what you mean.

    >>>      The problem is that the help/spec says nothing about the flag 'parallel' as being optional.

    >>>

    >>>

    >>>      I also asked this question:

    >>>        Q: What is going to happen if the resulting 'parallel' sub-string above is not a number?

    >>>

    >>>

    >>>      Thanks,

    >>>      Serguei

    >>>

    >>>

    >>>

    >>>              Thanks!

    >>>

    >>>

    >>>      BRs,

    >>>      Lin

    >>>

    >>>      From: mailto:serguei.spit...@oracle.com mailto:serguei.spit...@oracle.com

    >>>      Date: Friday, August 7, 2020 at 3:28 PM

    >>>      To: "linzang(臧琳)mailto:linz...@tencent.com,Hohensee, Paulmailto:hohen...@amazon.com,StefanKarlssonmailto:stefan.karls...@oracle.com,DavidHolmesmailto:david.hol...@oracle.com,serviceability-devmailto:serviceability-dev@openjdk.java.net,mailto:hotspot-gc-...@openjdk.java.netmailto:hotspot-gc-...@openjdk.java.netSubject:Re:RFR(L):8215624:addparallelheapinspectionsupportforjmaphisto(G1)(Internetmail)HiLin,Notsure,Ifullyunderstandthespecupdateandtheoptionsprocessinginthefile:http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.frames.htmlThespecsaysnothingifthenewoption'parallel'ismandatoryoroptional.Also,(itwasbeforeyourfix)thespecdoesnotsayiftheoptions'live'and'all'aremutuallyexclusive.TheJMap.javaimplementationjustprintusageintwocases:191}elseif(subopt.startsWith(parallel=")) {

    >>>       192                parallel = subopt.substring("parallel=".length());

    >>>       193                if (parallel == null) {

    >>>       194                     usage(1);

    >>>       195                }

    >>>       ...

    >>>       200         if (set_live && set_all) {

    >>>       201             usage(1);

    >>>       202         }

    >>>      It is not that helpful as the usage does not explain anything about these corner cases.

    >>>      Also, it allows to pass no parallel option.

    >>>      What is going to happen if null is passed in place of parallel here? :

    >>>       206         executeCommandForPid(pid, "inspectheap", liveopt, filename, parallel);

    >>>

    >>>      Should the lines 193-195 be moved after the line 202?

    >>>

    >>>      Thanks,

    >>>      Serguei

    >>>

    >>>

    >>>      On 8/5/20 18:59, linzang(臧琳) wrote:

    >>>      Thanks Paul!

    >>>      And I have verified this change could build success in windows.

    >>>

    >>>      BRs,

    >>>      Lin

    >>>

    >>>      On 2020/8/6, 4:17 AM, "Hohensee, Paulmailto:hohensee@amazon.comwrote:Twotinynitsthatdon'tneedanewwebrev:InheapInspection.cpp,youdon'tneedtocastmissed_counttouintxinthecalltolog_info().InheapInspection.hpp,youcandeletetwoofthethreeblanklinesbefore#endif//SHARE_MEMORY_HEAPINSPECTION_HPPThanks,PaulOn8/5/20,6:46AM,linzang(臧琳)" mailto:linz...@tencent.com wrote:

    >>>

    >>>              Hi Paul, Stefan and Serguei,

    >>>                  Here I uploaded a new changeset, would you like to help review again?

    >>>                  Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11/

    >>>                  Delta (based on webrev10): http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11_delta/

    >>>

    >>>                  P.S.  I am in process of building it on windows environment for a double check. May update result later. Thanks!

    >>>

    >>>

    >>>              BRs,

    >>>              Lin

    >>>

    >>>

    >>>

    >>>

    >>>

    >>

 

 


Reply via email to