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

[Chris Plummer]

On Thu, 10 Dec 2020 03:08:48 GMT, Lin Zang <lzang at openjdk.org> wrote:

>> 8257234 : Add gz option to SA jmap to write a gzipped heap dump
>
> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   delete unnecessary print

Changes requested by cjplummer (Reviewer).

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line 1776:

> 1774:             }
> 1775:         },
> 1776:         new Command("dumpheap", "dumpheap [gz=<1-9>] [filename]", false) {

There is a lot of replicated code in this command handler. I'd suggest checking for "gz=" first, regardless of the token count. Produce an error if it is missing and the token count is 2. Otherwise process it if present. Then fetch the filename argument.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java line 62:

> 60:         System.out.println("                             \tif gz specified, the heap dump is written");
> 61:         System.out.println("                             \tin gzipped format using the given compression level");
> 62:         System.err.println("                             \t1 (recommended) is the fastest, 9 the strongest compression.");

You need to address the fact that we now have help text that is multiple sentences. I don't like that there are no periods in this case (except you added one). And using periods implies that you should also start the sentence with an upper case letter. And if you are to do all this for this one option, then it should be done for all of them.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java line 158:

> 156:                             }
> 157:                             dumpfile = keyValue[1];
> 158:                         } else if (keyValue[0].equals("gz")) {

I don't see "gz" being rejected if "-heap" or "-heap:format=x" are used. I think it needs to be.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java line 203:

> 201:                 hgw = new HeapHprofBinWriter(gzLevel);
> 202:             } else {
> 203:                 System.err.println("Illegal compress level: " + gzLevel);

Should be "compression" level.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java line 130:

> 128:         System.out.println("    --binaryheap            To dump java heap in hprof binary format.");
> 129:         System.out.println("    --dumpfile <name>       The name of the dump file.");
> 130:         System.out.println("    --gz <1-9>              The compress level for gzipped dump file.");

Should be "compression" level.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java line 316:

> 314:            Map.entry("histo", "-histo"),
> 315:            Map.entry("clstats", "-clstats"),
> 316:            Map.entry("finalizerinfo", "-finalizerinfo"));

Indentation seems to be 3 instead of 4. And I think it's actually suppose to be 8 in this case.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 525:

> 523:     private void fillInHeapRecordLength() throws IOException {
> 524:         // For compression, the length is written by CompressedSegmentOutputStream
> 525:         if (isCompression()) return;

I would expect to find logic in `CompressedSegmentOutputStream` that is similar to what is being skipped here in `fillInHeapRecordLength()`, but I don't. For example, I don't see a check for overflow that results in an exception like there is a few lines below here.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1325:

> 1323:      * it behaves same as BufferedOutputStream.
> 1324:      * */
> 1325:     private class CompressedSegmentOutputStream extends BufferedOutputStream {

Is there a reason why `CompressedSegmentOutputStream` can't just be called `SegmentOutputStream` and always be enabled? That would simplify a lot of logic that currently is different based on whether or not you are compressing the output.

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

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