The CSR was just closed, so we're stuck with doing another one later.

Paul

On 2/11/19, 1:31 PM, "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> 
wrote:

    On 2/11/19 13:24, Hohensee, Paul wrote:
    > On " jmap -dump[:<dump-options>] <pid>", turns out that "jmap -dump" is 
illegal. A quick test produces an error (actually, the "Usage" message), and 
the source code confirms it.
    >
    >           for (String pid : pids) {
    >              if (pids.size() > 1) {
    >                  System.out.println("Pid:" + pid);
    >              }
    >              if (option.equals("-histo")) {
    >                  histo(pid, "");
    >              } else if (option.startsWith("-histo:")) {
    >                   histo(pid, option.substring("-histo:".length()));
    >              } else if (option.startsWith("-dump:")) {
    >                  dump(pid, option.substring("-dump:".length()));
    >             } else if (option.equals("-finalizerinfo")) {
    >                 executeCommandForPid(pid, "jcmd", "GC.finalizer_info");
    >             } else if (option.equals("-clstats")) {
    >                 executeCommandForPid(pid, "jcmd", "GC.class_stats");
    >             } else {
    >                 usage(1);
    >             }
    >
    > Should the spec and implementation be changed to allow "jmap -dump" as 
part of this fix?
    
    I'd say - Yes. It makes sense to align the two options to behave similarly.
    
    > I'd say yes, since it would require another CSR to do it separately.
    
    I'd suggest to use the same CSR to simplify things and minimize the 
    overhead.
    
    
    Thanks,
    Serguei
    
    >
    > Paul
    >
    > On 2/11/19, 1:11 PM, "serguei.spit...@oracle.com" 
<serguei.spit...@oracle.com> wrote:
    >
    >      On 2/11/19 12:53, Hohensee, Paul wrote:
    >      > In the comments in JMap.java, replace "... one of \"live\" and 
\"all\" ..." with "... one of \"live\" or \"all\" ...". I.e., replace "and" 
with "or".
    
    Forgot to say that the same update is also needed for dump-options.
    
    
    Thanks,
    Serguei
    
    >      
    >      > Also, "jmap -histo" is still a valid command, so change the 
comment " jmap -histo:<histo-options> <pid>" to " jmap -histo[:<histo-options>] 
<pid>".
    >      
    >      The same fix is needed in the -dump option:
    >         The "jmap -dump: <dump-options> <pid>" needs to be changed to " 
jmap
    >      -dump[:<dump-options>] <pid>".
    >      
    >      The CSR also needs the same update in the -dump and -histo options
    >      despite the fact it has been finalized.
    >      
    >      >
    >      > In BasicJMapTest.java, add tests for "-histo:all,file=" and 
"-histo:all".
    >      
    >      I was about to ask for the same.
    >      
    >      Thanks,
    >      Serguei
    >      
    >      > Thanks,
    >      >
    >      > Paul
    >      >
    >      > On 2/11/19, 4:55 AM, "臧琳" <zangl...@jd.com> wrote:
    >      >
    >      >      Dear All,
    >      >           Thanks your time for review.
    >      >           I have uploaded the refined webrev at  
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.07/.
    >      >           Would you like to help review again?
    >      >           And here are what I have done in this update.
    >      >           * Refined the spec of JMap -histo based on Joe's 
comments in the CSR https://bugs.openjdk.java.net/browse/JDK-8218131
    >      >           * Added the unit test for JMap based on Serguei's 
comments.
    >      >           * Fixed the comments format.
    >      >           Thanks !
    >      >
    >      >      Cheers,
    >      >      Lin
    >      >      ________________________________________
    >      >      From: 臧琳
    >      >      Sent: Saturday, February 9, 2019 18:52
    >      >      To: serguei.spit...@oracle.com; David Holmes; Hohensee, Paul; 
JC Beyler
    >      >      Cc: serviceability-dev@openjdk.java.net
    >      >      Subject: Re: [RFR]8215622: Add dump to file support for jmap 
histo
    >      >
    >      >      Thanks All for reviewing the change and CSR, I am still OOTO 
now and I will update the webrev later and ask your help for review again.
    >      >
    >      >      Happy Chinese New Year!
    >      >
    >      >      Cheers,
    >      >      Lin
    >      >      ________________________________________
    >      >      From: serguei.spit...@oracle.com <serguei.spit...@oracle.com>
    >      >      Sent: Wednesday, February 6, 2019 6:44:43 AM
    >      >      To: 臧琳; David Holmes; Hohensee, Paul; JC Beyler
    >      >      Cc: serviceability-dev@openjdk.java.net
    >      >      Subject: Re: [RFR]8215622: Add dump to file support for jmap 
histo
    >      >
    >      >      Hi Lin,
    >      >
    >      >      Thank you for the fix!
    >      >      It looks good in general.
    >      >
    >      >      Should it come with a unit test for new flags?
    >      >
    >      >      
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.06/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.frames.html
    >      >
    >      >
    >      >       154                 // get the canonical path - important to 
avoid just
    >      >       155                 // passing a "heap.bin" and having the 
dump created
    >      >       156                 // in the target VM working directory 
rather than the
    >      >       157                 // directory where jmap is executed.
    >      >
    >      >        Minor: Comment need to start with a capital letter: // Get 
the ...
    >      >
    >      >
    >      >      Thanks,
    >      >      Serguei
    >      >
    >      >      On 1/30/19 10:42 PM, 臧琳 wrote:
    >      >
    >      >      Hi David, Paul,
    >      >          I have uploaded the refined webrev at
    >      >          http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.06/
    >      >          And submitted a CSR at 
https://bugs.openjdk.java.net/browse/JDK-8218131
    >      >          May I ask your help to review?
    >      >          Thanks!
    >      >
    >      >      BRs,
    >      >      Lin
    >      >      ________________________________________
    >      >      From: David Holmes 
<david.hol...@oracle.com><mailto:david.hol...@oracle.com>
    >      >      Sent: Thursday, January 31, 2019 9:19:31 AM
    >      >      To: Hohensee, Paul; 臧琳; JC Beyler
    >      >      Cc: 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>
    >      >      Subject: Re: [RFR]8215622: Add dump to file support for jmap 
histo
    >      >
    >      >      On 31/01/2019 10:41 am, Hohensee, Paul wrote:
    >      >
    >      >
    >      >      Also, I suspect that this change needs a CSR, since we're 
adding functionality to jmap's histo switch. Opinions?
    >      >
    >      >
    >      >
    >      >      Yes. Changes to command-line flags need a CSR.
    >      >
    >      >      Thanks,
    >      >      David
    >      >
    >      >
    >      >
    >      >      Thanks,
    >      >
    >      >      Paul
    >      >
    >      >      On 1/30/19, 9:33 AM, "serviceability-dev on behalf of 
Hohensee, Paul" <serviceability-dev-boun...@openjdk.java.net on behalf of 
hohen...@amazon.com><mailto:serviceability-dev-boun...@openjdk.java.netonbehalfofhohen...@amazon.com>
 wrote:
    >      >
    >      >           A nit in TestLoggerWeakRefLeak.java, separate the 2 
arguments to heapHisto() by a space.
    >      >
    >      >           vm.heapHisto("", "-live")
    >      >
    >      >           Also, the header comment line for 
getInstanceCountFromHeapHisto() should be changed to
    >      >
    >      >                * 'vm.heapHisto("", "-live")' will request a full GC
    >      >
    >      >           In attachListener.cpp, the line
    >      >
    >      >                 out->print_cr("Invalid argument to dumpheap 
operation: %s", arg1);
    >      >
    >      >           should be
    >      >
    >      >                 out->print_cr("Invalid argument to inspectheap 
operation: %s", arg1);
    >      >
    >      >           Otherwise looks fine. The two failing tests (the other 
one was test/jdk/java/util/concurrent/locks/Lock/TimedAcquireLeak.java) now 
pass on my mac laptop.
    >      >
    >      >           Paul
    >      >
    >      >           On 1/30/19, 12:18 AM, "臧琳" 
<zangl...@jd.com><mailto:zangl...@jd.com> wrote:
    >      >
    >      >               Dear All,
    >      >                   This issue mentioned is that test case 
"java/util/logging/TestLoggerWeakRefLeak.java" Failed after applied the webrev.
    >      >                   I have identified the root cause of the issue. 
it is caused by 2 problems.
    >      >                    1. The path processing in heap_inspection() at 
attachListener.cpp.
    >      >                        The problem is that when use jmap 
-histo:live , the path is actually set to "" when it is transfer to socket. so 
it cause JNI_ERR.
    >      >                        I need to modify the logic here that if 
path[0] == '\0' , fall through to the next argument parsing, rather than return 
JNI_ERR.
    >      >
    >      >                    2. Another problem is 
HotSpotVirtualMachine.java, it has a heapHisto() method, and the testcase use 
vm.heapHisto("-live"), And after the patch applied, it should be 
vm.heapHisto(""/*filepath*/, "-live"), seems we need to change the API for 
handling it.
    >      >
    >      >                    I have upload a webrev05:
    >      >                    
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.05/
    >      >
    >      >               May I ask your help for review?
    >      >
    >      >               Thanks,
    >      >               Lin
    >      >               ________________________________________
    >      >               From: 臧琳
    >      >               Sent: Monday, January 28, 2019 5:49:42 PM
    >      >               To: Hohensee, Paul; JC Beyler
    >      >               Cc: 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>
    >      >               Subject: Re: [RFR]8215622: Add dump to file support 
for jmap histo
    >      >
    >      >               Dear all,
    >      >                    I have found there is problem for handling the 
"filepath" and it cause test "java/util/logging/TestLoggerWeakRefLeak.java" 
fail, I have identified the root cause, please wait for the new update.
    >      >                   Thanks!
    >      >
    >      >               BRs,
    >      >               Lin
    >      >               ________________________________________
    >      >               From: 臧琳
    >      >               Sent: Friday, January 25, 2019 9:00:19 AM
    >      >               To: Hohensee, Paul; JC Beyler
    >      >               Cc: 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>
    >      >               Subject: Re: [RFR]8215622: Add dump to file support 
for jmap histo
    >      >
    >      >               Dear All,
    >      >                       May I get more review about this webrev?
    >      >                       
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
    >      >
    >      >               Thanks!
    >      >               BRs,
    >      >               Lin
    >      >               ________________________________________
    >      >               From: 臧琳
    >      >               Sent: Tuesday, January 22, 2019 2:18:06 PM
    >      >               To: Hohensee, Paul; JC Beyler
    >      >               Cc: 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>
    >      >               Subject: Re: [RFR]8215622: Add dump to file support 
for jmap histo
    >      >
    >      >               Hi Paul,
    >      >                   Thanks a lot for your review. I have refined it 
based on your comments.
    >      >                   
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.04/
    >      >                   Would you like to help review it again? Thanks
    >      >
    >      >               BRs,
    >      >               Lin
    >      >               ________________________________________
    >      >               From: Hohensee, Paul 
<hohen...@amazon.com><mailto:hohen...@amazon.com>
    >      >               Sent: Friday, January 18, 2019 6:11:14 AM
    >      >               To: 臧琳; JC Beyler
    >      >               Cc: 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>
    >      >               Subject: Re: [RFR]8215622: Add dump to file support 
for jmap histo
    >      >
    >      >               Just a few small things.
    >      >
    >      >               In attachListener.cpp, line 278, is the static_cast 
needed? fileStream is a subclass of outputStream, so it should be ok to pass to 
the VM_GC_Heap_Inspection constructor, but maybe there's some C++ arcana I 
don't know about.
    >      >
    >      >               In attachListener.cpp, line 275, change "Fail to" to 
"Failed to".
    >      >
    >      >               In JMap.java, line 286, change      use \"all\""    
to    use \"all\"."    to match line 277.
    >      >
    >      >               Thanks,
    >      >
    >      >               Paul
    >      >
    >      >               On 1/11/19, 1:39 AM, "臧琳" 
<zangl...@jd.com><mailto:zangl...@jd.com> wrote:
    >      >
    >      >                   Hi Jc, Paul and All,
    >      >                        Here is webrev03 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.03/
    >      >                        would you like to help review?
    >      >
    >      >                   Thanks,
    >      >                   Lin
    >      >                   ________________________________________
    >      >                   From: 臧琳
    >      >                   Sent: Friday, January 11, 2019 10:25:27 AM
    >      >                   To: JC Beyler
    >      >                   Cc: Hohensee, Paul; 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>
    >      >                   Subject: 答复: [RFR]8215622: Add dump to file 
support for jmap histo
    >      >
    >      >                   Hi Jc,
    >      >                        Thanks a lot. it is not a necessary change, 
I forget to omit it:)
    >      >
    >      >                   BRs,
    >      >                   Lin
    >      >                   ________________________________
    >      >                   发件人: JC Beyler 
<jcbey...@google.com><mailto:jcbey...@google.com>
    >      >                   发送时间: 2019年1月11日 0:58:22
    >      >                   收件人: 臧琳
    >      >                   抄送: Hohensee, Paul; 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>
    >      >                   主题: Re: [RFR]8215622: Add dump to file support 
for jmap histo
    >      >
    >      >                   Hi Lin,
    >      >
    >      >                   Small nit:
    >      >                   
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.02/src/java.base/share/native/libjli/java.c.udiff.html
    >      >
    >      >                   needs a copyright update,
    >      >                   Jc
    >      >
    >      >
    >      >                   On Wed, Jan 9, 2019 at 7:48 PM 臧琳 
<zangl...@jd.com<mailto:zangl...@jd.com><mailto:zangl...@jd.com><mailto:zangl...@jd.com>>
 wrote:
    >      >                   Dear All,
    >      >                          I have updated the refined webrev at 
http://cr.openjdk.java.net/~xiaofeya/8215622/webrev.02/
    >      >                          Would you like to help review? Thanks!
    >      >
    >      >                   BRs,
    >      >                   Lin
    >      >                   From: 臧琳
    >      >                   Sent: Wednesday, January 9, 2019 11:00 AM
    >      >                   To: 'JC Beyler' 
<jcbey...@google.com<mailto:jcbey...@google.com><mailto:jcbey...@google.com><mailto:jcbey...@google.com>>
    >      >                   Cc: Hohensee, Paul 
<hohen...@amazon.com<mailto:hohen...@amazon.com><mailto:hohen...@amazon.com><mailto:hohen...@amazon.com>>;
 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net>
    >      >                   Subject: RE: [RFR]8215622: Add dump to file 
support for jmap histo
    >      >
    >      >                   Dear JC,
    >      >                          Thanks to point it out, I processed the 
“-file=” case in JMap.java but forgot to do it in attachListener.cpp. I will do 
it in next webrev.
    >      >
    >      >                   Cheers,
    >      >                   Lin
    >      >
    >      >                   From: JC Beyler 
<jcbey...@google.com<mailto:jcbey...@google.com><mailto:jcbey...@google.com><mailto:jcbey...@google.com>>
    >      >                   Sent: Wednesday, January 9, 2019 10:51 AM
    >      >                   To: 臧琳 
<zangl...@jd.com<mailto:zangl...@jd.com><mailto:zangl...@jd.com><mailto:zangl...@jd.com>>
    >      >                   Cc: Hohensee, Paul 
<hohen...@amazon.com<mailto:hohen...@amazon.com><mailto:hohen...@amazon.com><mailto:hohen...@amazon.com>>;
 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net>
    >      >                   Subject: Re: [RFR]8215622: Add dump to file 
support for jmap histo
    >      >
    >      >                   Hi Lin,
    >      >
    >      >                   Inlined as well :-)
    >      >
    >      >                   On Tue, Jan 8, 2019 at 6:23 PM 臧琳 
<zangl...@jd.com<mailto:zangl...@jd.com><mailto:zangl...@jd.com><mailto:zangl...@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 
<jcbey...@google.com<mailto:jcbey...@google.com><mailto:jcbey...@google.com><mailto:jcbey...@google.com>>
    >      >                   Sent: Wednesday, January 9, 2019 1:42 AM
    >      >                   To: 臧琳 
<zangl...@jd.com<mailto:zangl...@jd.com><mailto:zangl...@jd.com><mailto:zangl...@jd.com>>
    >      >                   Cc: Hohensee, Paul 
<hohen...@amazon.com<mailto:hohen...@amazon.com><mailto:hohen...@amazon.com><mailto:hohen...@amazon.com>>;
 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@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 臧琳 
<zangl...@jd.com<mailto:zangl...@jd.com><mailto:zangl...@jd.com><mailto:zangl...@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-boun...@openjdk.java.net<mailto:serviceability-dev-boun...@openjdk.java.net><mailto:serviceability-dev-boun...@openjdk.java.net><mailto:serviceability-dev-boun...@openjdk.java.net>>
 代表 臧琳 
<zangl...@jd.com<mailto:zangl...@jd.com><mailto:zangl...@jd.com><mailto:zangl...@jd.com>>
    >      >                   发送时间: 2019年1月3日 10:21
    >      >                   收件人: Hohensee, Paul; 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@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 
<hohen...@amazon.com<mailto:hohen...@amazon.com><mailto:hohen...@amazon.com><mailto:hohen...@amazon.com>>
    >      >                   Sent: Saturday, December 29, 2018 3:54 AM
    >      >                   To: 臧琳 
<zangl...@jd.com<mailto:zangl...@jd.com><mailto:zangl...@jd.com><mailto:zangl...@jd.com>>;
 
serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@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-boun...@openjdk.java.net<mailto:serviceability-dev-boun...@openjdk.java.net><mailto:serviceability-dev-boun...@openjdk.java.net><mailto:serviceability-dev-boun...@openjdk.java.net>>
 on behalf of 臧琳 
<zangl...@jd.com<mailto:zangl...@jd.com><mailto:zangl...@jd.com><mailto:zangl...@jd.com>>
    >      >                   Date: Thursday, December 20, 2018 at 11:03 PM
    >      >                   To: 
"serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net>"
 
<serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-dev@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
    >      >
    >      >
    >      >                   --
    >      >
    >      >                   Thanks,
    >      >                   Jc
    >      >
    >      >
    >      >
    >      >
    >      >
    >      >
    >      >
    >      >
    >      >
    >      >
    >      
    >      
    >
    
    

Reply via email to