[RFR]8215622: Add dump to file support for jmap histo
JC Beyler
jcbeyler at google.com
Wed Jan 9 02:50:42 UTC 2019
Hi Lin,
Inlined as well :-)
On Tue, Jan 8, 2019 at 6:23 PM 臧琳 <zanglin5 at jd.com> wrote:
> Dear JC,
>
> Thanks for your comments, I inlined my comments here:
>
>
> http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/src/hotspot/share/services/attachListener.cpp.udiff.html
>
> - Should we do like the rest of the file and declare variables when
> needed instead of doing them all at the start?
>
> --- (Lin) I will do that in next webrev.
>
> - Should the method return JNI_ERR if the file cannot be created
> (because if not, then why fail if no file is passed at all?)
>
> --- (Lin) The logic is that when user use “-file=<filename>”, the file
> must be created successful, otherwise the “-file=” not work, which break
> user’s expection, so it fail here. If user not specify “-file=”, it
> indicate that user not expect to dump to file, so the outputStream will be
> used. Do you think it is reasonable?
>
>
>
No that is reasonable BUT your code currently allows the user to do
"--file="; in this absurd case, your code prints out "No dump file
specified" and just continues. Why not make that fail as well?
> The bigger issue I see is the passing of NULL for a filename, why do we
> not do things where you just really pass "-file=<file>" to the
> attachListener.cpp and handle the parsing there?; it would then make more
> sense to me: we either pass "-file=<file>" or not but we no longer have a
> "maybe there is or not a file, so maybe there is a NULL there".
>
> ---(Lin) This is similar with what I have done in webrev00, but I think
> maybe processing arguments in JMap.java is more reasonable, I think
> logically, it is JMap’s responsibility to parsed it’s command line
> arguments, and then pass it to attachListener. The attachListener just
> hearing from the socket and get command and parsed arguments. And one more
> reason maybe that in java it is easy to get the canonical path from the API
> getCanonicalPath(), which I guess maybe a little complicate to do it cross
> platform in attachListener.cpp.
>
I think it's a style choice perhaps? I'd rather have the code look at the
arguments and see if it is --file or if it is --live or --all and then
figure out what to do instead of having now "null or a file" for arg0. But
I can see the conversation go both ways in this case.
Thanks!
Jc
>
>
> All other comments will be handled in the next webrev. Thanks a lot for
> your review and suggestions.
>
>
>
> Cheers,
>
> Lin
>
>
>
>
>
> *From:* JC Beyler <jcbeyler at google.com>
> *Sent:* Wednesday, January 9, 2019 1:42 AM
> *To:* 臧琳 <zanglin5 at jd.com>
> *Cc:* Hohensee, Paul <hohensee at amazon.com>;
> serviceability-dev at openjdk.java.net
> *Subject:* Re: [RFR]8215622: Add dump to file support for jmap histo
>
>
>
> Hi Lin,
>
>
>
> I have a few nits:
>
>
> http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java.udiff.html
>
>
> http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java.udiff.html
>
>
> http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java.udiff.html
>
>
>
> You could fix the spaces arount the for loop you changed:
>
>
>
> + for (int i=0; i<4; i++) {
>
> to
>
>
>
> + for (int i = 0; i < 4; i++) {
>
>
>
>
>
>
> http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/src/hotspot/share/services/attachListener.cpp.udiff.html
>
> - Should we do like the rest of the file and declare variables when
> needed instead of doing them all at the start?
>
> - Should the method return JNI_ERR if the file cannot be created
> (because if not, then why fail if no file is passed at all?)
>
>
>
>
> http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html
>
> + filename = opt.substring(5);
>
> + if (filename != null) {
>
> -> I don't see how filename can be null here
>
> -> same filename could just be declared in the if
>
>
>
>
>
>
>
> + } else if (subopt.startsWith("file=")) {
>
> + filename = parseFileName(subopt);
>
>
>
> -> So actually you are testing twice if the string starts with "file=",
> maybe remove the one in the method?
>
>
>
> -> in the option string printouts, you don't specify that all is an option
> as well, is that normal?
>
>
>
>
>
> The bigger issue I see is the passing of NULL for a filename, why do we
> not do things where you just really pass "-file=<file>" to the
> attachListener.cpp and handle the parsing there?; it would then make more
> sense to me: we either pass "-file=<file>" or not but we no longer have a
> "maybe there is or not a file, so maybe there is a NULL there".
>
>
>
> Thanks,
>
> Jc
>
>
>
> On Mon, Jan 7, 2019 at 2:11 AM 臧琳 <zanglin5 at jd.com> wrote:
>
> Hi Paul,
>
> I think it is not necessary to have a loop for argument processing,
> since there is not so much arguments to handle.
>
> And I made a new webrev
> http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/
>
>
>
> Hi All,
>
> May I also ask your help for reviewing this webrev?
>
> webrev http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.01/
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215622
>
>
>
> Thanks!
>
> Lin
>
>
> ------------------------------
>
> *发件人**:* serviceability-dev <serviceability-dev-bounces at openjdk.java.net>
> 代表 臧琳 <zanglin5 at jd.com>
> *发送时间**:* 2019年1月3日 10:21
> *收件人**:* Hohensee, Paul; serviceability-dev at openjdk.java.net
> *主题**:* [发件地址伪造,恶意邮件,勿点] RE: [RFR]8215622: Add dump to file support for
> jmap histo
>
>
>
> Dear Paul,
>
> Thanks a lot for your review! I am trying to remend the patch
>
> One problem is that in future, I will add more options for histo,
> such as “parallel”, “incremental”.
>
> so do you thinking option processing with loop in heap_inspection() in
> attachListener.cpp would
>
> be more reasonable than the “if else” ?
>
> Thanks
>
>
>
> BRs,
>
> Lin
>
>
>
>
>
>
>
> *From:* Hohensee, Paul <hohensee at amazon.com>
> *Sent:* Saturday, December 29, 2018 3:54 AM
> *To:* 臧琳 <zanglin5 at jd.com>; serviceability-dev at openjdk.java.net
> *Subject:* Re: [RFR]8215622: Add dump to file support for jmap histo
>
>
>
> Update the copyright dates to 2019 please, since this won’t get pushed
> until then.
>
>
>
> In JMap.java, I’d emulate dump() and use
>
>
>
> String filename = null;
>
> String subopts[] = options.split(",");
>
>
>
> for (String subopt : subopts){
>
> if (subopt.isEmpty() || subopt.equals("all")) {
>
> // pass
>
> } else if (subopt.equals("live")) {
>
> liveopt = "-live";
>
> } else if (subopt.startsWith("file=")) {
>
> // file=<file> - check that <file> is specified
>
> if (subopt.length() > 5) {
>
> filename = subopt.substring(5);
>
> }
>
> } else {
>
> usage(1);
>
> }
>
> }
>
>
>
> // get the canonical path - important to avoid just passing
>
> // a "heap.bin" and having the dump created in the target VM
>
> // working directory rather than the directory where jmap
>
> // is executed.
>
> filename = new File(filename).getCanonicalPath();
>
> // inspectHeap is not the same as jcmd GC.class_histogram
>
> executeCommandForPid(pid, "inspectheap", filename, liveopt);
>
>
>
> I.e., use an enhanced for loop to scan the array, and duplicate dump()’s
> executeCommandForPid() argument order, as well as dump()’s “file=<>” check
> (the code that starts with “if (subopt.startsWith”) and canonicalization.
> Actually, better to factor the latter out into its own method and use it
> from both histo() and dump().
>
>
>
> The argument checking code in heap_inspection() in attachListener.cpp can
> be simplified along the lines of dump_heap(). I.e., you don’t need to loop
> over the argument list. To match up with dump_heap()’s info messages, the
> info message string at the end should be “Heap inspection file created: %s”.
>
>
>
> Thanks,
>
>
>
> Paul
>
>
>
> *From: *serviceability-dev <serviceability-dev-bounces at openjdk.java.net>
> on behalf of 臧琳 <zanglin5 at jd.com>
> *Date: *Thursday, December 20, 2018 at 11:03 PM
> *To: *"serviceability-dev at openjdk.java.net" <
> serviceability-dev at openjdk.java.net>
> *Subject: *[RFR]8215622: Add dump to file support for jmap histo
>
>
>
> Hi All,
>
> May I ask your help to review this patch for enhance jmap –histo.
>
> It add the “file=<path>” arguments that allow jmap –histo outputs data to
> file directly.
>
> This patch is also part of the enhancement described in
> https://bugs.openjdk.java.net/browse/JDK-8214535.
>
>
>
> Webrev: http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.00/
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215622
>
>
>
> Thanks.
>
>
>
> BRs,
>
> Lin
>
>
>
>
>
>
>
>
>
>
> --
>
>
>
> Thanks,
>
> Jc
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190108/46a60b32/attachment-0001.html>
More information about the serviceability-dev
mailing list