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 >> >> >> >> >> > >