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

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

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

Lin Zang-2
On Tue, 9 Feb 2021 08:28:57 GMT, Serguei Spitsyn <[hidden email]> wrote:

>> 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

Dear @sspitsyn,
Thanks for your suggestion.

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

No problem, please take your time. Appreciated for your effort helping review!

BRs,
Lin

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

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