On Thu, 10 Dec 2020 03:08:48 GMT, Lin Zang <lz...@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

Reply via email to