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>
Sent: Thursday, January 31, 2019 9:19:31 AM
To: Hohensee, Paul; 臧琳; JC Beyler
Cc: 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> 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> 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
> 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
> 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
> 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>
> Sent: Friday, January 18, 2019 6:11:14 AM
> To: 臧琳; JC Beyler
> Cc: 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> 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
> 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>
> 发送时间: 2019年1月11日 0:58:22
> 收件人: 臧琳
> 抄送: Hohensee, Paul; 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>> 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>>
> Cc: Hohensee, Paul
> <hohen...@amazon.com<mailto:hohen...@amazon.com>>;
> 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>>
> Sent: Wednesday, January 9, 2019 10:51 AM
> To: 臧琳 <zangl...@jd.com<mailto:zangl...@jd.com>>
> Cc: Hohensee, Paul
> <hohen...@amazon.com<mailto:hohen...@amazon.com>>;
> 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>> 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>>
> Sent: Wednesday, January 9, 2019 1:42 AM
> To: 臧琳 <zangl...@jd.com<mailto:zangl...@jd.com>>
> Cc: Hohensee, Paul
> <hohen...@amazon.com<mailto:hohen...@amazon.com>>;
> 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>> 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>>
> 代表 臧琳 <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>
> 主题: [发件地址伪造,恶意邮件,勿点] 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>>
> Sent: Saturday, December 29, 2018 3:54 AM
> To: 臧琳 <zangl...@jd.com<mailto:zangl...@jd.com>>;
> 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>>
> on behalf of 臧琳 <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>"
>
> <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
>
>
>
>
>
>
