[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: [RFR]8215623: Add incremental dump for jmap histo
From:       <zanglin5 () jd ! com>
Date:       2019-04-22 2:05:39
Message-ID: 7c7f6ab507b0471baad13de89141c365 () jd ! com
[Download RAW message or body]

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@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@google.com<mailto:jcbeyler@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@jd.com<mailto:zanglin5@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@jd.com<mailto:zanglin5@jd.com>> дµÀ£º

Sorry, it should be CSR ticket

Thanks£¬
Lin

ÔÚ 2019Äê4Ô 11ÈÕ£¬ÉÏÎç11:16£¬ê°ÁÕ <zanglin5@jd.com<mailto:zanglin5@jd.com>> дµÀ£º

Also realized it requires a JCP ticket, I will create it.


Cheers,
Lin

ÔÚ 2019Äê4Ô 11ÈÕ£¬ÉÏÎç10:26£¬ê°ÁÕ <zanglin5@jd.com<mailto:zanglin5@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@google.com<mailto:jcbeyler@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@jd.com<mailto:zanglin5@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@google.com<mailto:jcbeyler@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@jd.com<mailto:zanglin5@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@jd.com<mailto:zanglin5@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@amazon.com<mailto:hohensee@amazon.com>>; serviceability-
dev@openjdk.java.net<mailto:dev@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<mailto: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 <hohensee@amazon.com<mailto:hohensee@amazon.com>>; serviceability-
dev@openjdk.java.net<mailto:dev@openjdk.java.net>; ê°ÁÕ \
                <zanglin5@jd.com<mailto:zanglin5@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@amazon.com<mailto:hohensee@amazon.com><mailto:hohensee@amazon.com<mailto:hohensee@amazon.com>>>; \
serviceability- dev@openjdk.java.net<mailto:dev@openjdk.java.net><mailto:serviceability-dev@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
<hohensee@amazon.com<mailto:hohensee@amazon.com><mailto:hohensee@amazon.com<mailto:hohensee@amazon.com>>>
 ·¢ËÍʱ¼ä: 2019Äê1Ô 1ÈÕ 4:56
ÊÕ¼þÈË: ê°ÁÕ; serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-<mailto:serviceability->
 dev@openjdk.java.net<mailto:dev@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@openjdk.java.net<mailto:bounces@openjdk.java.net><mailto:serviceability-dev-<mailto:serviceability-dev->
 bounces@openjdk.java.net<mailto:bounces@openjdk.java.net>>> on behalf of ê°ÁÕ
<zanglin5@jd.com<mailto:zanglin5@jd.com><mailto:zanglin5@jd.com<mailto:zanglin5@jd.com>>>
                
Date: Thursday, December 20, 2018 at 11:13 PM
To: "serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net><mailto:serviceability-<mailto:serviceability->
 dev@openjdk.java.net<mailto:dev@openjdk.java.net>>" <serviceability-
dev@openjdk.java.net<mailto:dev@openjdk.java.net><mailto:serviceability-dev@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 ¨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


[prev in list] [next in list] [prev in thread] [next in thread]