[RFR]8215622: Add dump to file support for jmap histo
臧琳
zanglin5 at jd.com
Sat Feb 9 10:52:56 UTC 2019
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.spitsyn at oracle.com <serguei.spitsyn at oracle.com>
Sent: Wednesday, February 6, 2019 6:44:43 AM
To: 臧琳; David Holmes; Hohensee, Paul; JC Beyler
Cc: serviceability-dev at 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.holmes at oracle.com><mailto:david.holmes at oracle.com>
Sent: Thursday, January 31, 2019 9:19:31 AM
To: Hohensee, Paul; 臧琳; JC Beyler
Cc: serviceability-dev at openjdk.java.net<mailto:serviceability-dev at 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 at openjdk.java.net on behalf of hohensee at amazon.com><mailto:serviceability-dev-bounces at openjdk.java.netonbehalfofhohensee@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 at jd.com><mailto:zanglin5 at 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 at openjdk.java.net<mailto:serviceability-dev at 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 at openjdk.java.net<mailto:serviceability-dev at 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 at openjdk.java.net<mailto:serviceability-dev at 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 at amazon.com><mailto:hohensee at amazon.com>
Sent: Friday, January 18, 2019 6:11:14 AM
To: 臧琳; JC Beyler
Cc: serviceability-dev at openjdk.java.net<mailto:serviceability-dev at 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 at jd.com><mailto:zanglin5 at 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 at openjdk.java.net<mailto:serviceability-dev at 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 at google.com><mailto:jcbeyler at google.com>
发送时间: 2019年1月11日 0:58:22
收件人: 臧琳
抄送: Hohensee, Paul; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at 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 at jd.com<mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com><mailto:zanglin5 at 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 at google.com<mailto:jcbeyler at google.com><mailto:jcbeyler at google.com><mailto:jcbeyler at google.com>>
Cc: Hohensee, Paul <hohensee at amazon.com<mailto:hohensee at amazon.com><mailto:hohensee at amazon.com><mailto:hohensee at amazon.com>>; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-dev at 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 at google.com<mailto:jcbeyler at google.com><mailto:jcbeyler at google.com><mailto:jcbeyler at google.com>>
Sent: Wednesday, January 9, 2019 10:51 AM
To: 臧琳 <zanglin5 at jd.com<mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com>>
Cc: Hohensee, Paul <hohensee at amazon.com<mailto:hohensee at amazon.com><mailto:hohensee at amazon.com><mailto:hohensee at amazon.com>>; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-dev at 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 at jd.com<mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com><mailto:zanglin5 at 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 at google.com<mailto:jcbeyler at google.com><mailto:jcbeyler at google.com><mailto:jcbeyler at google.com>>
Sent: Wednesday, January 9, 2019 1:42 AM
To: 臧琳 <zanglin5 at jd.com<mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com>>
Cc: Hohensee, Paul <hohensee at amazon.com<mailto:hohensee at amazon.com><mailto:hohensee at amazon.com><mailto:hohensee at amazon.com>>; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-dev at 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 at jd.com<mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com><mailto: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<mailto:serviceability-dev-bounces at openjdk.java.net><mailto:serviceability-dev-bounces at openjdk.java.net><mailto:serviceability-dev-bounces at openjdk.java.net>> 代表 臧琳 <zanglin5 at jd.com<mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com>>
发送时间: 2019年1月3日 10:21
收件人: Hohensee, Paul; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-dev at openjdk.java.net><mailto: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<mailto:hohensee at amazon.com><mailto:hohensee at amazon.com><mailto:hohensee at amazon.com>>
Sent: Saturday, December 29, 2018 3:54 AM
To: 臧琳 <zanglin5 at jd.com<mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com><mailto:zanglin5 at jd.com>>; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-dev at openjdk.java.net><mailto: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><mailto: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><mailto: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><mailto: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><mailto: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 –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
More information about the serviceability-dev
mailing list