[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo
From: 臧琳 <zanglin5 () jd ! com>
Date: 2019-01-31 6:42:33
Message-ID: 4c797f80a87748c5bb260ea5715be75e () jd ! com
[Download RAW message or body]
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.holmes@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-bounces@openjdk.java.net on behalf of hohensee@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, "臧琳" <zanglin5@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 <hohensee@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, "臧琳" <zanglin5@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 <jcbeyler@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 臧琳 <zanglin5@jd.com<mailto:zanglin5@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' <jcbeyler@google.com<mailto:jcbeyler@google.com>>
> Cc: Hohensee, Paul <hohensee@amazon.com<mailto:hohensee@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 <jcbeyler@google.com<mailto:jcbeyler@google.com>>
> Sent: Wednesday, January 9, 2019 10:51 AM
> To: 臧琳 <zanglin5@jd.com<mailto:zanglin5@jd.com>>
> Cc: Hohensee, Paul <hohensee@amazon.com<mailto:hohensee@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 臧琳 <zanglin5@jd.com<mailto:zanglin5@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@google.com<mailto:jcbeyler@google.com>>
> Sent: Wednesday, January 9, 2019 1:42 AM
> To: 臧琳 <zanglin5@jd.com<mailto:zanglin5@jd.com>>
> Cc: Hohensee, Paul <hohensee@amazon.com<mailto:hohensee@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 臧琳 <zanglin5@jd.com<mailto:zanglin5@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@openjdk.java.net<mailto:serviceability-dev-bounces@openjdk.java.net>> \
> 代表 臧琳 <zanglin5@jd.com<mailto:zanglin5@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 <hohensee@amazon.com<mailto:hohensee@amazon.com>>
> Sent: Saturday, December 29, 2018 3:54 AM
> To: 臧琳 <zanglin5@jd.com<mailto:zanglin5@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-bounces@openjdk.java.net<mailto:serviceability-dev-bounces@openjdk.java.net>> \
> on behalf of 臧琳 <zanglin5@jd.com<mailto:zanglin5@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
>
>
>
>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic