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
