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.
Okay, I see what you mean.
>> 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.
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: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
Date: Friday, August 7, 2020 at 3:28 PM
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,
Not sure, I fully understand the spec update and the options processing in the file:
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.frames.html
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.
The JMap.java implementation just print usage in two cases:191 } else if (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,LinOn 2020/8/6, 4:17 AM, "Hohensee, Paul" <hohen...@amazon.com> wrote:Two tiny nits that don't need a new webrev:In heapInspection.cpp, you don't need to cast missed_count to uintx in the call to log_info().In heapInspection.hpp, you can delete two of the three blank lines before #endif // SHARE_MEMORY_HEAPINSPECTION_HPPThanks,PaulOn 8/5/20, 6:46 AM, "linzang(臧琳)" <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
