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

[Lin Zang]

On Tue, 26 Jan 2021 10:03:13 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Copyrights need updating.
>
> Minor suggestion for `src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java`:
> +                        } else if (keyValue[0].equals("gz")) {
> +                            if (keyValue.length == 1) {
> +                                System.err.println("Argument is expected for "gz"");
> +                                System.exit(1);
> +                            }
> +                            String level = keyValue[1];
> +                            if (mode == MODE_HEAP_GRAPH_GXL) {
> +                                System.err.println(""gz" option is not compatible with heap dump in GXL format");
> +                                System.exit(1);
> +                            }
> +                            . . .
> The check of MODE_HEAP_GRAPH_GXL is better to move above the check ` if (keyValue.length == 1)`.

Hi @sspitsyn, 

> I did not understand your answer on the code below (it seems the problem existed before your fix):
> 
>                          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";
>                           }
> There is a check for gzlevel == 0 when it has been already found this is true: gzlevel < 1. It just has no logical sense. I think, the code is broken and has to do something like this:
> 
>                         } else if (cntTokens == 1) {
>                               filename = option;
>                              // Try to parse "gz=" option. If failed, treat it as filename.
>                              if (option.startsWith("gz=")) {
>                                  gzlevel = parseHeapDumpCompressionLevel(option);
>                                  if (gzlevel > 0 && gzlevel <= 9) {
>                                      // Got correct gzlevel, use default filename.
>                                      filename = "heap.bin.gz";
>                                  } else {
>                                      err.println("Compression level not in range: "" + option + "".");
>                                      usage();
>                                      return;
>                                  }
>                             }
>                         }
> 
Let me try to explain and then maybe we can have more talk.  
The gzLevel after `parseHeapDumpCompressionLevel(option)` can be -1, 0, and 1-9. so if it is < 1,  it could be 0 for out-of-range number, and -1 for other non-number value. 

The reason for this is that I think the dumpheap command could face two kind of scenario: 
1. `dumpheap gz=abc.hprof`   which  seems strange but it could be a legal filename for heapdump. so in this case, it outputs `use "gz=abc.hprof" option as output and saves the collected data in to this file. 
 
2. `dumpheap gz=100` which I'd like to treat as a wrong compression level passed to gz option, so it exits with usage prompted.

Do you think it is reasonable to treat "gz=[number]/[non-number]" differently in this case?  or should it just exit with error for all "gz=" options that is not in the range of compression level? BTW, I think your suggested code is better if we consider all illegal compression level as an error. 

Thanks,
Lin

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

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