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
