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, "[email protected]"
> <[email protected]> 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: "[email protected]" <[email protected]>
>> Date: Tuesday, August 11, 2020 at 8:40 AM
>> To: "linzang(臧琳)" <[email protected]>, "Hohensee, Paul"
>> <[email protected]>, Stefan Karlsson <[email protected]>, David
>> Holmes <[email protected]>, serviceability-dev
>> <[email protected]>, "[email protected]"
>> <[email protected]>
>> 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:[email protected] 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:[email protected] mailto:[email protected]
>> Date: Tuesday, August 11, 2020 at 5:11 AM
>> To: "linzang(臧琳)" mailto:[email protected], "Hohensee, Paul"
>> mailto:[email protected], Stefan Karlsson
>> mailto:[email protected], David Holmes
>> mailto:[email protected], serviceability-dev
>> mailto:[email protected],
>> mailto:[email protected] mailto:[email protected]
>> 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:[email protected] mailto:[email protected]
>> Date: Friday, August 7, 2020 at 3:28 PM
>> To: "linzang(臧琳)mailto:[email protected],Hohensee,
>> Paulmailto:[email protected],StefanKarlssonmailto:[email protected],DavidHolmesmailto:[email protected],serviceability-devmailto:[email protected],mailto:[email protected]:[email protected]: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:[email protected]:Twotinynitsthatdon'tneedanewwebrev:InheapInspection.cpp,youdon'tneedtocastmissed_counttouintxinthecalltolog_info().InheapInspection.hpp,youcandeletetwoofthethreeblanklinesbefore#endif//SHARE_MEMORY_HEAPINSPECTION_HPPThanks,PaulOn8/5/20,6:46AM,linzang(臧琳)"
>> mailto:[email protected] 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
>>
>>
>>
>>
>>
>
>
