[RFR]8215622: Add dump to file support for jmap histo
JC Beyler
jcbeyler at google.com
Tue Jan 8 17:42:01 UTC 2019
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190108/9c6bbdb6/attachment-0001.html>
More information about the serviceability-dev
mailing list