[RFR]8215623: Add incremental dump for jmap histo
Jean Christophe Beyler
jcbeyler at google.com
Wed Apr 3 20:57:53 UTC 2019
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> 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> 写道：
>
> 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>; serviceability-
> 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
> 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>; serviceability-
> dev at openjdk.java.net; 臧琳 <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>>; 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>>
> 发送时间: 2019年1月1日 4:56
> 收件人: 臧琳; serviceability-dev at openjdk.java.net<mailto:serviceability-
> 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:serviceability-dev-
> bounces at openjdk.java.net>> on behalf of 臧琳
> <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>" <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 –histo.
>
> It adds the “incremental” arguments that allow jmap –histo 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190403/590e0fef/attachment-0001.html>
More information about the serviceability-dev
mailing list