On Tue, 19 Jan 2021 12:39:12 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:
>
> fix issue of setting gz option with no value
I think you need to add some additional testing. You can use
ClhsdbDumpheap.java and also BasicJMapTest.java. Besides testing with a
compressed hprof file, also add a test for "gz=" with no compression value and
with invalid compression value.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line
1792:
> 1790: return;
> 1791: }
> 1792: if (keyValue[0].equals("gz")) {
This code now assumes there will be a `gz` option specified. If there isn't,
you will still execute this section of code, and get an NPE at this point.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 403:
> 401: VM vm = VM.getVM();
> 402:
> 403: // Check weather we should dump the heap as segments
Should be "whether"
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 475:
> 473: if (!useSegmentedHeapDump) {
> 474: // Fill in final length
> 475: fillInHeapRecordLength();
It's unclear to me why this code is now conditional on not using a segmented
heap dump.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 1307:
> 1305:
> 1306: /**
> 1307: * The class implements a buffered output stream for segment data
> dump.
Should be "segmented data dump"
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 1309:
> 1307: * The class implements a buffered output stream for segment data
> dump.
> 1308: * It is used inside HeapHprofBinWritter only for heap dump.
> 1309: * Because the current implementation of segment heap dump needs to
> update
"segmented"
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 1316:
> 1314: * If the data to be written are larger than internal buffer, or
> the internal buffer
> 1315: * is full, the internal buffer will be extend to a larger one.
> 1316: * This class defines a switch to turn on/off the segment mode. if
> turned off,
"segmented". Also, uppercase "If".
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 1317:
> 1315: * is full, the internal buffer will be extend to a larger one.
> 1316: * This class defines a switch to turn on/off the segment mode. if
> turned off,
> 1317: * it behaves same as BufferedOutputStream.
"the same as"
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 517:
> 515:
> 516: private void fillInHeapRecordLength() throws IOException {
> 517: assert !useSegmentedHeapDump : "fillInHeapRecordLength is not
> supported for segment heap dump";
"segmented heap dump"
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 1321:
> 1319: private class SegmentedOutputStream extends BufferedOutputStream {
> 1320: /**
> 1321: * Creates a new buffered output stream to support segment heap
> dump data.
"segmented"
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 1335:
> 1333:
> 1334: /**
> 1335: * Creates a new buffered output stream to support segment heap
> dump data.
"segmented"
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java
line 1493:
> 1491: // Segment support.
> 1492: private boolean segmentMode;
> 1493: private boolean allowSegmental;
"allowSegmented" would be a better name.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1712
