RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v9]

[Lin Zang]

On Tue, 26 Jan 2021 08:50:06 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line 2119:
>> 
>>> 2117:             }
>>> 2118:         } else {
>>> 2119:             err.println("Unknow option \"" + option + "\"");
>> 
>> "Unknown"
>
> -                if (t.countTokens() > 1) {
> +                if (t.countTokens() > 2) {
>                      usage();
>                  } else {
>                      JMap jmap = new JMap();
> -                    String filename;
> -                    if (t.countTokens() == 1) {
> -                        filename = t.nextToken();
> -                    } else {
> -                        filename = "heap.bin";;
> +                    String filename = "heap.bin";
> +                    int gzlevel = -1;
> +                    int cntTokens = t.countTokens();
> I'd suggest to move the last line above to the beginning of method and use it in first comparison as well to make it more consistent.
> 
> It'd be nice to make the comments more consistent.
> For instance:
> `+                         * possible command:`
>  Start it from capital letter:  `Possible`.
>  
>  Replace:
> +                            /* first argument is compression level, second is filename */
> +                            /* Parse "gz=" option. */
> with:
> +                            /* First argument is compression level, second is filename. */
> +                            * Parse "gz=" option. */
> 
> Replace:
> `+                            // Try to parse "gz=" option, if failed, treat it as filename`
> with:
> `+                            // Try to parse "gz=" option. If failed, treat it as filename.`
> 
> Start from capital letter:
> `+                                      // compression level not in range.`
> 
> 
> It'd be nice to reduce the following:
> +                    if (cntTokens > 0) {
> +                        String option = t.nextToken();
> +                        /*
> +                         * possible command:
> +                         *     dumpheap gz=1 file;
> +                         *     dumpheap gz=1;
> +                         *     dumpheap file;
> +                         */
> 
> to:
> +                    String option = t.nextToken();
> +                    /*
> +                     * Possible command:
> +                     *     dumpheap gz=1 file;
> +                     *     dumpheap gz=1;
> +                     *     dumpheap file;
> +                     *     dumpheap;
> +                     */
> 
> It does not seem the condition `if (cntTokens > 0) {`  is very useful.
> 
> The checks below seems to be incorrect:
> +                              **if (gzlevel < 1)** {
> +                                  err.println("Can not parse compression level from option "" + option + "".");
> +                                  **if (gzlevel == 0)** {
> +                                      // compression level not in range.
> +                                      usage();
> +                                      return;
> +                                  } else {
> +                                      out.println("Use "" + option +"" as dumped file name.");
> +                                  }
> +                              } else {
> +                                  filename = "heap.bin.gz";
> +                              }
> 
> At least, how the gzlevel can be equal to 0 if it was already fount it is below 0?
> 
> It seems, Chris has already asked a question about checking the max compress level limit.

Hi @sspitsyn,
> It does not seem the condition if (cntTokens > 0) { is very useful.
 
This is used to guard the case cntTokens == 0, otherwise that t.nextToken() will throw ArrayIndexOutOfBoundsException.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712