On Mon, 25 Jan 2021 13:02:56 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 coding style issue
Copyrights need updating.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line
2119:
> 2117: }
> 2118: } else {
> 2119: err.println("Unknow option \"" + option + "\"");
"Unknown"
test/hotspot/jtreg/serviceability/sa/ClhsdbDumpheap.java line 50:
> 48: public class ClhsdbDumpheap {
> 49: private static final String kHeapDumpFileNameDefault = "heap.bin";
> 50: private static final String kHeapDumpFileNameGzDefault =
> "heap.bin.gz";
I'm not sure about the naming here. Why the `k` prefix? Usually in java static
final variables are all upper case with `_` separating the words. So maybe
something like HEAPDUMP_FILENAME_DEFAULT
test/hotspot/jtreg/serviceability/sa/ClhsdbDumpheap.java line 73:
> 71: File out = new File(deCompressedFile);
> 72: try {
> 73: GZIPInputStream gis = new GZIPInputStream(new
> FileInputStream(dump));
`printStackTraces()` uses `Reader.getStack()`, which does not know about
gzipped hprof files. However, `Reader.readFile()` was modified to automatically
detect gzipped hprof files. I suggest you do the same for `getStack()` rather
than making the test do all the work. Probably `getStack()` and `readFile()`
can share the code that does this.
test/hotspot/jtreg/serviceability/sa/ClhsdbDumpheap.java line 95:
> 93: boolean needVerify;
> 94:
> 95: public SubTest(String comm, String fName, String expected,
> boolean isComp, boolean verify) {
I think moving the `expected` argument to the last position would make it
easier to read all the invocations of `SubTest()`
test/hotspot/jtreg/serviceability/sa/ClhsdbDumpheap.java line 190:
> 188: for (int i = 0; i < subtests.length;i++) {
> 189: runTest(theApp.getPid(), subtests[i]);
> 190: }
Nice job organizing all the test cases!
-------------
PR: https://git.openjdk.java.net/jdk/pull/1712
