RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Aug 11 03:04:38 UTC 2020
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.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
> Date: Tuesday, August 11, 2020 at 8:40 AM
> To: "linzang(臧琳)" <linzang at tencent.com>, "Hohensee, Paul" <hohensee at amazon.com>, Stefan Karlsson <stefan.karlsson at oracle.com>, David Holmes <david.holmes at oracle.com>, serviceability-dev <serviceability-dev at openjdk.java.net>, "hotspot-gc-dev at openjdk.java.net" <hotspot-gc-dev at 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:linzang at 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.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com
> Date: Tuesday, August 11, 2020 at 5:11 AM
> To: "linzang(臧琳)" mailto:linzang at tencent.com, "Hohensee, Paul" mailto:hohensee at amazon.com, Stefan Karlsson mailto:stefan.karlsson at oracle.com, David Holmes mailto:david.holmes at oracle.com, serviceability-dev mailto:serviceability-dev at openjdk.java.net, mailto:hotspot-gc-dev at openjdk.java.net mailto:hotspot-gc-dev at 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.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com
> Date: Friday, August 7, 2020 at 3:28 PM
> To: "linzang(臧琳)mailto:linzang at tencent.com,Hohensee, Paulmailto:hohensee at amazon.com,StefanKarlssonmailto:stefan.karlsson at oracle.com,DavidHolmesmailto:david.holmes at oracle.com,serviceability-devmailto:serviceability-dev at openjdk.java.net,mailto:hotspot-gc-dev at openjdk.java.netmailto:hotspot-gc-dev at 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 at 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:linzang at 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
>
>
>
>
>
More information about the serviceability-dev
mailing list