[RFR]8215623: Add incremental dump for jmap histo
臧琳
zanglin5 at jd.com
Mon Apr 22 02:05:39 UTC 2019
Dear All,
May I ask for more review of this webrev and CSR?
Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215623/webrev.06/
CSR: https://bugs.openjdk.java.net/browse/JDK-8222319.
Thanks!
Lin
________________________________________
From: ê°ÁÕ
Sent: Monday, April 15, 2019 10:54
To: Jean Christophe Beyler
Cc: ê°ÁÕ; Hohensee, Paul; serviceability-dev at openjdk.java.net
Subject: Re: [RFR]8215623: Add incremental dump for jmap histo
Dear Jc,
Thanks a lot for your suggestions, I have updated the CSR.
Hi All,
May I get more review about the webrev and CSR,
Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215623/webrev.06/
CSR: https://bugs.openjdk.java.net/browse/JDK-8222319.
BRs,
Lin
ÔÚ 2019Äê4ÔÂ13ÈÕ£¬ÉÏÎç2:15£¬Jean Christophe Beyler <jcbeyler at google.com<mailto:jcbeyler at google.com>> дµÀ£º
Hi Lin,
You could state in the CSR:
- That not using the new flags makes the system behave as before; so it is a nop if you don't use the new flags
- What happens if the flags are passed negative values?
- What happens if the flags are passed crazy big numbers?
Finally, I'd put a link to the current webrev if someone wanted more information.
Apart from that, it looks good to me (I might skip the diff as you put the output below) the text could just say: here is the new usage text (maxchunk and maxfilesize are new)
Thanks,
Jc
On Wed, Apr 10, 2019 at 11:56 PM ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>> wrote:
Dear All,
I have created a CSR at https://bugs.openjdk.java.net/browse/JDK-8222319.
May I ask your help to review it? Thanks!
BRs,
Lin
ÔÚ 2019Äê4ÔÂ11ÈÕ£¬ÉÏÎç11:22£¬ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>> дµÀ£º
Sorry, it should be CSR ticket
Thanks£¬
Lin
ÔÚ 2019Äê4ÔÂ11ÈÕ£¬ÉÏÎç11:16£¬ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>> дµÀ£º
Also realized it requires a JCP ticket, I will create it.
Cheers,
Lin
ÔÚ 2019Äê4ÔÂ11ÈÕ£¬ÉÏÎç10:26£¬ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>> дµÀ£º
Dear JC,
Thanks so much, I suddenly realized your way is exactly what I want.
And here is new wehbrev. http://cr.openjdk.java.net/~lzang/jmap-8214535/8215623/webrev.06/
BRs,
Lin
ÔÚ 2019Äê4ÔÂ11ÈÕ£¬ÉÏÎç4:40£¬Jean Christophe Beyler <jcbeyler at google.com<mailto:jcbeyler at google.com>> дµÀ£º
Hi Lin,
What I meant about the helper method was to not do this:
+ private static String add_option(String cmd, String opt) {
+ if (opt.equals("")) {
+ return cmd;
+ }
+ return cmd + opt + ",";
+ }
+
but to do this:
+ private static String add_option(String cmd, String opt) {
+ if (cmd.isEmpty()) {
+ return opt;
+ }
+ return cmd + "," + opt;
+ }
+
That way you only put the ',' when needed,
Jc
On Wed, Apr 10, 2019 at 5:33 AM ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>> wrote:
Dear Jc,
Thanks a lot for your comments, here is the refined webrev. May I ask your help to review again?
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215623/webrev.05/
BRs,
Lin
ÔÚ 2019Äê4ÔÂ4ÈÕ£¬ÉÏÎç4:57£¬Jean Christophe Beyler <jcbeyler at google.com<mailto:jcbeyler at google.com>> дµÀ£º
Hi Lin,
Just a few nits to be honest:
- http://cr.openjdk.java.net/~lzang/jmap-8214535/8215623/webrev.04/src/hotspot/share/utilities/ostream.cpp.udiff.html
-> I don't think it's a good idea to just pass a char* and implicitly imagine it is 256 in length
-> Should we not add that length as a parameter? btw, the test you have len > 255 is a bit too restrictive no? Technically you should wait until you find the last '/' and then test there no?
(technically we could use strrchr, no?)
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215623/webrev.04/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html
-> the adding of the "," seems a bit off; I think it would be smarter to just have a helper add_option to cmdline that checks if it is not empty, then add "," and the new option
-> otherwise you always end the "," if there are more options even if you are ignoring them, no?
Finally:
} else if (subopt.equals("live")) {
- liveopt = "-live";
+ // Add '-' for compatibility.
+ cmdline += "-" + subopt;
for consistency with how you do it for "all", should this not be:
} else if (subopt.equals("live")) {
- liveopt = "-live";
+ // Add '-' for compatibility.
+ cmdline += "-live";
Thanks,
Jc
On Mon, Apr 1, 2019 at 5:18 AM ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>> wrote:
Dear All£¬
Here I updated the latest changeset which did the following refine:
* fixed the compatibility issues so that old version of jmap can work normally with latest changeset.
* revised the code for parsing the jmap histo arguments.
* revised the logic for incremental dumping of jmap histo.
The main change of this webrev is making all arguments passed to virtual machine as one line. So there is no need to care about the max argument count limitation. And hence resolved the compatibility issues as discussed in http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027338.html
May I ask your help to review?
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215623/webrev.04/
BRs,
Lin
ÔÚ 2019Äê2ÔÂ26ÈÕ£¬ÉÏÎç10:50£¬ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>> дµÀ£º
Dear All,
I have revised the webrev at http://cr.openjdk.java.net/~xiaofeya/8215623/webrev.02/.
May I ask your help for reviewing? Thanks
BRs,
Lin
-----Original Message-----
From: ê°ÁÕ
Sent: Friday, January 25, 2019 9:01 AM
To: Hohensee, Paul <hohensee at amazon.com<mailto:hohensee at amazon.com>>; serviceability-
dev at openjdk.java.net<mailto:dev at openjdk.java.net>
Subject: Re: [RFR]8215623: Add incremental dump for jmap histo
Dear All,
May I get more review about this webrev?
Webrev: http://cr.openjdk.java.net/~xiaofeya/8215623/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215623
Thanks!
Lin
________________________________________
From: ê°ÁÕ
Sent: Wednesday, January 9, 2019 9:46:55 AM
To: Hohensee, Paul; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>
Subject: RE: [RFR]8215623: Add incremental dump for jmap histo
Hi Paul and All,
Thanks a lot for your review and comments, I have updated the webrev to
Webrev: http://cr.openjdk.java.net/~xiaofeya/8215623/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215623
Would you like to help review it. Thanks.
BRs
Lin
From: ê°ÁÕ
Sent: Monday, January 7, 2019 7:14 PM
To: Hohensee, Paul <hohensee at amazon.com<mailto:hohensee at amazon.com>>; serviceability-
dev at openjdk.java.net<mailto:dev at openjdk.java.net>; ê°ÁÕ <zanglin5 at jd.com<mailto:zanglin5 at jd.com>>
Subject: RE: [RFR]8215623: Add incremental dump for jmap histo
And another way is to add a argument ¡°IncrementalFile=<path>¡±,which
specifies the location for saving the intermediate data file. And if it is not
specified, incremental data will dump to ¡°out¡±.
What do you think?
Thanks,
Lin
From: ê°ÁÕ
Sent: Monday, January 7, 2019 7:02 PM
To: 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:dev at openjdk.java.net><mailto:serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>>
Subject: ´ð¸´: [RFR]8215623: Add incremental dump for jmap histo
Dear Paul,
Thanks for your review, I agree your suggestion to make incremental
general. and
I think the incremental file is better to be different with the file or the
"out", because
in future the parallel histo will have each thread individually dump their data,
and I want
it to be lock-free, so each thread need to dump to their own file. The final
data
will be sum up and dump to the file that "filename=<file>" specified.
Moreover, the incremental
file contains intermediate data, if it is dumped to "<file>", it more or less
"pollute" the final file (final file that contains lots intermediate data).
so the logic is that incremental data will be dumped to a file named
"Histo_Dump_Temp.dump",
if the "filename" is assigned, the "Histo_Dump_Temp.dump" will be
generated under the same folder,
if "filename" not specified, it will dump to current dir. And if "chunkcount" is 0
or max_int, or "maxfilesize" is 0, the incremental dump will be disabled.
________________________________
·¢¼þÈË: Hohensee, Paul
<hohensee at amazon.com<mailto:hohensee at amazon.com><mailto:hohensee at amazon.com<mailto:hohensee at amazon.com>>>
·¢ËÍʱ¼ä: 2019Äê1ÔÂ1ÈÕ 4:56
ÊÕ¼þÈË: ê°ÁÕ; serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-<mailto:serviceability->
dev at openjdk.java.net<mailto:dev at openjdk.java.net>>
Ö÷Ìâ: Re: [RFR]8215623: Add incremental dump for jmap histo
As for 8215622, update the copyright dates to 2019 please, since this won¡¯t
get pushed until then.
You might generalize the implementation so that all inspections are done
incrementally, and parameterize RecordInstanceClosure with the incremental
threshold. ¡°incremental¡± could become ¡°chunkcount=<n>¡±, where <n>
defaults to infinity (max value of size_t).
I¡¯d not use a default file name when ¡°chunkcount¡± is specified, I¡¯d just write to
whatever the output stream is. ¡°chunkcount¡± is then independent of ¡°file¡±.
I¡¯d add another histo argument for the maximum file size, call it ¡°maxfilesize¡±,
and parameterize RecordInstanceClosure with it. Default would be infinity
(max value of size_t).
If you want to make it easy to use your 8k and 5mb chunkcount and
maxfilesize combination, you could redefine your original ¡°incremental¡±
argument as syntactic sugar for ¡°chunkcount=8k,maxfilesize=5m¡±.
INCREMENTAL_THRESHOLD and MAX_INCREMENTAL_FILESIZE become
DEFAULT_CHUNKSIZE and MAX_FILE_SIZE, are both set to max size_t, and
should be defined as ¡°const int¡± within RecordInstanceClosure.
Thanks,
Paul
From: serviceability-dev <serviceability-dev-
bounces at openjdk.java.net<mailto:bounces at openjdk.java.net><mailto:serviceability-dev-<mailto:serviceability-dev->
bounces at openjdk.java.net<mailto: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:13 PM
To: "serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net><mailto:serviceability-<mailto:serviceability->
dev at openjdk.java.net<mailto:dev at openjdk.java.net>>" <serviceability-
dev at openjdk.java.net<mailto:dev at openjdk.java.net><mailto:serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>>>
Subject: [RFR]8215623: Add incremental dump for jmap histo
Hi All,
May I ask your help to review this patch for enhance jmap ¨Chisto.
It adds the ¡°incremental¡± arguments that allow jmap ¨Chisto to incrementally
save the intermediate data into a temp file.
The intermediate data is dumped incrementally and write to a rolling file,
which limit the size of the temp file to be small.
This is useful for user to get intermediate results when jmap/jvm process is
killed accidentally. Especially when the heap is large.
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/8215623/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215623
Thanks.
BRs,
Lin
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
More information about the serviceability-dev
mailing list