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 臧琳 <zangl...@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 <jcbey...@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 臧琳 <zangl...@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，臧琳 <zangl...@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 <hohen...@amazon.com>; serviceability-
>> d...@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@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 <hohen...@amazon.com>; serviceability-
>> d...@openjdk.java.net; 臧琳 <zangl...@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
>> <hohen...@amazon.com<mailto:hohen...@amazon.com>>; serviceability-
>> d...@openjdk.java.net<mailto:serviceability-dev@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
>> <hohen...@amazon.com<mailto:hohen...@amazon.com>>
>> 发送时间: 2019年1月1日 4:56
>> 收件人: 臧琳; serviceability-dev@openjdk.java.net<mailto:serviceability-
>> d...@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-
>> 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:13 PM
>> To: "serviceability-dev@openjdk.java.net<mailto:serviceability-
>> d...@openjdk.java.net>" <serviceability-
>> d...@openjdk.java.net<mailto:serviceability-dev@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
>
>
>
--
Thanks,
Jc
