答复: [RFR]8215622: Add dump to file support for jmap histo

[臧琳]

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<mailto:serviceability-dev-bounces at openjdk.java.net>> on behalf of ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>>
Date: Thursday, December 20, 2018 at 11:03 PM
To: "serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>" <serviceability-dev at openjdk.java.net<mailto: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 ¨Chisto.

It add the ¡°file=<path>¡± arguments that allow jmap ¨Chisto 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






-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190107/08d1ded1/attachment.html>