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.

Dear @sspitsyn, 
Thanks for reviewing!  
The following change:
>                              gzlevel = parseHeapDumpCompressionLevel(option)
>                               **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";
>                               }

The logic here is to handle corner case that use "gz=" as a filename. IMO, dumpheap could work with one argument, and if that argument is not "gz=[number]", it should be treated as filename. 
E.g: "dumpheap gz=", should work to generate a dump file named "gz=", and with some warning message like "Can not parse compression level from option gz=".  
And if the argument is like "gz=10", which is out of range, it will be treated as a wrong gziplevel option and the command will print usage and exit. 

So the code `gzlevel = parseHeapDumpCompressionLeve(option)` is designed to reture 3 type of values:
>  1-9  -- legal comression level
>  0      --  compression level is out of range
> -1     --  other cases, e.g. compression level is not a number. 
Then the above code could process different cases.
P.S.  Found that comments at the definition of parseHeapDumpCompressionLevel() is wrong, maybe it mislead you, sorry for that, and I will fix it at next update.

Thanks,
Lin

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

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