RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v17]

[Serguei Spitsyn]

On Tue, 9 Feb 2021 06:37:21 GMT, Lin Zang <lzang at openjdk.org> wrote:

>> Hi Lin,
>> 
>> Sorry for confusing you but I've not noticed a duplication in your code:
>> 
>> 1778                 if (cntTokens > 2) {
>> 1779                     usage();
>> 1780                 } else {
>> 1781                     JMap jmap = new JMap();
>> 1782                     String filename = "heap.bin";
>> 1783                     int gzlevel = 0;
>>  . . . . 
>> 1794                     if (cntTokens > 2) {
>> 1795                         err.println("More than 2 options specified: " + cntTokens);
>> 1796                         usage();
>> 1797                         return;
>> 1798                     }
>> 1799                     if (cntTokens == 1) { // first argument could be filename or "gz="
>>                              . . . .
>> 
>> There are two checks for cntTokens > 2. The second check has to be removed.
>> Also, a return needs to be added after the line 1779 with "usage();" , so the "else" statement can be removed.
>> This has to be refactored to something like this:
>> 1778                 if (cntTokens > 2) {
>>                              err.println("More than 2 options specified: " + cntTokens);
>> 1779                     usage();
>>                              return;
>> 1780                 }
>> 1781                 JMap jmap = new JMap();
>> 1782                 String filename = "heap.bin";
>> 1783                 int gzlevel = 0;
>> 1799                 if (cntTokens == 1) { // first argument could be filename or "gz="
>>                          . . . .
>> 
>> Thanks,
>> Serguei
>
> Dear Serguei,
> 
>> There are two checks for cntTokens > 2. The second check has to be removed.
>> Also, a return needs to be added after the line 1779 with "usage();" , so the "else" statement can be removed.
>> This has to be refactored to something like this:
> 
> Nice catch and good suggestion. Thanks a lot for point it out!
> I think It is no hurry to push this PR, and welcome for any comment.
> Thanks!

Hi Lin,
Thank you for update.

test/lib/jdk/test/lib/hprof/parser/Reader.java:
+            } else if ((i >>> 8) == 0x1f8b08) {
+                // Possible gziped file.

Could you, please, define a named constant and use it instead of the literal value 0x1f8b08?
Also, would it make sense to extend the comment to explain about it a little bit?

I'm still looking at some files (e.g., HeapHprofBinWriter.java).

Thanks,
Serguei

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712