On Tue, 26 Jan 2021 10:03:13 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Copyrights need updating.
>
> Minor suggestion for
> `src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java`:
> + } else if (keyValue[0].equals("gz")) {
> + if (keyValue.length == 1) {
> + System.err.println("Argument is expected for
> "gz"");
> + System.exit(1);
> + }
> + String level = keyValue[1];
> + if (mode == MODE_HEAP_GRAPH_GXL) {
> + System.err.println(""gz" option is not
> compatible with heap dump in GXL format");
> + System.exit(1);
> + }
> + . . .
> The check of MODE_HEAP_GRAPH_GXL is better to move above the check ` if
> (keyValue.length == 1)`.
Hi @sspitsyn,
> I did not understand your answer on the code below (it seems the problem
> existed before your fix):
>
> gzlevel = parseHeapDumpCompressionLevel(option)
> if (gzlevel < 1) {
> err.println("Can not parse compression level
> from option "" + option + "".");
> if (gzlevel == 0) {
> // compression level not in range.
> usage();
> return;
> } else {
> out.println("Use "" + option +"" as dumped
> file name.");
> }
> } else {
> filename = "heap.bin.gz";
> }
> There is a check for gzlevel == 0 when it has been already found this is
> true: gzlevel < 1. It just has no logical sense. I think, the code is broken
> and has to do something like this:
>
> } else if (cntTokens == 1) {
> filename = option;
> // Try to parse "gz=" option. If failed, treat
> it as filename.
> if (option.startsWith("gz=")) {
> gzlevel =
> parseHeapDumpCompressionLevel(option);
> if (gzlevel > 0 && gzlevel <= 9) {
> // Got correct gzlevel, use default
> filename.
> filename = "heap.bin.gz";
> } else {
> err.println("Compression level not in
> range: "" + option + "".");
> usage();
> return;
> }
> }
> }
>
Let me try to explain and then maybe we can have more talk.
The gzLevel after `parseHeapDumpCompressionLevel(option)` can be -1, 0, and
1-9. so if it is < 1, it could be 0 for out-of-range number, and -1 for other
non-number value.
The reason for this is that I think the dumpheap command could face two kind of
scenario:
1. `dumpheap gz=abc.hprof` which seems strange but it could be a legal
filename for heapdump. so in this case, it outputs `use "gz=abc.hprof" option
as output and saves the collected data in to this file.
2. `dumpheap gz=100` which I'd like to treat as a wrong compression level
passed to gz option, so it exits with usage prompted.
Do you think it is reasonable to treat "gz=[number]/[non-number]" differently
in this case? or should it just exit with error for all "gz=" options that is
not in the range of compression level? BTW, I think your suggested code is
better if we consider all illegal compression level as an error.
Thanks,
Lin
-------------
PR: https://git.openjdk.java.net/jdk/pull/1712
