RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump

Mon Apr 20 12:12:54 UTC 2020

Hi, I don't want to review this but could you put this new code in its 
own file?  heapDumper only needs CompressionBackend to be exported, from 
what I can tell.

Thanks,
Coleen

On 4/20/20 6:12 AM, Reingruber, Richard wrote:
> Hi Ralf,
>
>>> 767: I think _current->in_used doesn't take the final 9 bytes into account that are written in
>>> DumperSupport::end_of_dump() after the last dump segment has been finished.
>>> You could call get_new_buffer() instead of the if clause.
>> Wow, how did you found this? I've fixed it by making sure we flush the DumpWriter before calling the deactivate method.
> Spending long hours on the review ;)
> Ok with the fix.
>
>>> ### src/java.base/share/native/libzip/zip_util.c
>>> 1610: Will be hard to beat zlib_block_alloc() and zlib_block_free() performance wise. But have you
>>>   measured the performance gain? In other words: is it worth it? :)
>> This is not done for performance, but to make sure the allocation will not fail midway during writing the dump. Maybe it is not worth it, though.
> Understood. The heap dump will succeed if you can allocate at least one WriteWork instance. Without
> that you could get out of memory errors in the zlib which would make the dump fail. Ok!
>
>> http://cr.openjdk.java.net/~rschmelter/webrevs/8237354/webrev.2/
> Thanks for the clarifications and the changes in the new webrev.
> Webrev.2 looks good to me.
>
> Cheers, Richard.
>
> -----Original Message-----
> From: Schmelter, Ralf <ralf.schmelter at sap.com>
> Sent: Montag, 20. April 2020 10:14
> To: Reingruber, Richard <richard.reingruber at sap.com>; Ioi Lam <ioi.lam at oracle.com>; Langer, Christoph <christoph.langer at sap.com>; Yasumasa Suenaga <suenaga at oss.nttdata.com>; serguei.spitsyn at oracle.com; hotspot-runtime-dev at openjdk.java.net runtime <hotspot-runtime-dev at openjdk.java.net>
> Cc: serviceability-dev at openjdk.java.net
> Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
>
> Hi Richard,
>
> thanks for the review. I have incorporated your remarks into a new webrev:
> http://cr.openjdk.java.net/~rschmelter/webrevs/8237354/webrev.2/
>
> Some remarks to specific points:
>
>> ### src/hotspot/share/services/heapDumper.cpp
>> 762: assert(_active, "Must be active");
>>
>> It appears to me that the assertion would fail, if an error occurred creating the CompressionBackend.
> You are supposed to check for errors after creating the DumpWriter (which creates the CompressionBackend). And in case of an error, you directly destruct the object. I've added a comment to make that clear.
>
>> 767: I think _current->in_used doesn't take the final 9 bytes into account that are written in
>> DumperSupport::end_of_dump() after the last dump segment has been finished.
>> You could call get_new_buffer() instead of the if clause.
> Wow, how did you found this? I've fixed it by making sure we flush the DumpWriter before calling the deactivate method.
>
>> 1064: DumpWriter::DumpWriter()
>>
>> There doesn't seem to be enough error handling if _buffer cannot be allocated.
>> E.g. DumpWriter::write_raw() at line 1091 will enter an endless loop.
> As described above, this will not happen if we check for error after constructing the DumpWriter.
>
>> ### src/java.base/share/native/libzip/zip_util.c
>> 1610: Will be hard to beat zlib_block_alloc() and zlib_block_free() performance wise. But have you
>>   measured the performance gain? In other words: is it worth it? :)
> This is not done for performance, but to make sure the allocation will not fail midway during writing the dump. Maybe it is not worth it, though.
>
>> 1655: The result of deflateBound() seems to depend on the header comment, which is not given
>> here. Could this be an issue, because ZIP_GZip_Fully() can take a comment?
> I've added a 1024 byte additional bytes to avoid the problem.
>
>> ### test/lib/jdk/test/lib/hprof/parser/Reader.java
>>
>> 93: is the created GzipRandomAccess instance closed somewhere?
> The object is not closed since it is still used by the Snapshot returned.
>
> Best regard,
> Ralf
>
>
> -----Original Message-----
> From: Reingruber, Richard <richard.reingruber at sap.com>
> Sent: Tuesday, 14 April 2020 10:30
> To: Schmelter, Ralf <ralf.schmelter at sap.com>; Ioi Lam <ioi.lam at oracle.com>; Langer, Christoph <christoph.langer at sap.com>; Yasumasa Suenaga <suenaga at oss.nttdata.com>; serguei.spitsyn at oracle.com; hotspot-runtime-dev at openjdk.java.net runtime <hotspot-runtime-dev at openjdk.java.net>
> Cc: serviceability-dev at openjdk.java.net
> Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
>
> Hi Ralf,
>
> thanks for providing this enhancement to parallel gzip-compress heap dumps!
>
> I reckon it's safe to say that the coding is sophisticated. It would be awesome if you could sketch
> the idea of how HeapDumper, DumpWriter and CompressionBackend work together to produce the gzipped
> dump in a source code comment. Just enough to get started if somebody should ever have to track down
> a bug -- an unlikely event, I know ;)
>
> Please find the details of my review below.
>
> Thanks, Richard.
> // Not Reviewer
>
> --
>
> ### src/hotspot/share/services/diagnosticCommand.cpp
>
> 510   _gzip_level("-gz-level", "The compression level from 0 (store) to 9 (best) when writing in gzipped format.",
> 511               "INT", "FALSE", "1") {
>
>      "FALSE" should be probably false.
>
> ### src/hotspot/share/services/diagnosticCommand.hpp
> Ok.
>
> ### src/hotspot/share/services/heapDumper.cpp
>
> 390: Typo: initized
>
> 415: Typo: GZipComressor
>
> 477: Could you please add a comment, how the "HPROF BLOCKSIZE" comment is helpful?
>
> 539: Member variables of WriteWork are missing the '_' prefix.
>
> 546: Just a comment: WriteWork::in_max is actually a compile time constant. Would be nice if it could be
>       declared so. One could use templates for this, but then my favourite ide (eclipse cdt) doesn't
>       show me references and call hierarchies anymore. So I don't think it is worth it.
>
> 591: Typo: Removes the first element. Returns NULL is empty.
>
> 663: _writer, _compressor, _lock could be const.
>
> 762: assert(_active, "Must be active");
>
>       It appears to me that the assertion would fail, if an error occurred creating the CompressionBackend.
>
> 767: I think _current->in_used doesn't take the final 9 bytes into account that are written in
>       DumperSupport::end_of_dump() after the last dump segment has been finished.
>       You could call get_new_buffer() instead of the if clause.
>
> 903: Typo: Check if we don not waste more than _max_waste
>
> 1064: DumpWriter::DumpWriter()
>
>       There doesn't seem to be enough error handling if _buffer cannot be allocated.
>       E.g. DumpWriter::write_raw() at line 1091 will enter an endless loop.
>
> 2409: A comment, why Shenandoah is not supported, would be good.
>        In general I'd say it is good and natural to use the GC work threads.
>
> ### src/hotspot/share/services/heapDumper.hpp
> Ok.
>
> ### src/java.base/share/native/libzip/zip_util.c
>
> I'm not familiar with zlib, but here are my .02€ :)
>
> 1610: Will be hard to beat zlib_block_alloc() and zlib_block_free() performance wise. But have you
>        measured the performance gain? In other words: is it worth it? :)
>
> 1655: The result of deflateBound() seems to depend on the header comment, which is not given
>        here. Could this be an issue, because ZIP_GZip_Fully() can take a comment?
>
> 1658: deflateEnd() should not be called if deflateInit2Wrapper() failed. I think this can lead
>        otherwise to a double free() call.
>
> ### test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpCompressedTest.java
>
> 66: Maybe additionally check the exit value?
>
> 73: It's unclear to me, why this fails. Because the dump already exists? Because the level is
>      invalid? Reading the comment I'd expect success, not failure.
>
> ### test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpCompressedTestEpsilon.java
> Ok.
>
> ### test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpCompressedTestShenandoah.java
> Ok.
>
> ### test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpCompressedTestZ.java
> Ok.
>
> ### test/lib/jdk/test/lib/hprof/parser/GzipRandomAccess.java
> Ok.
>
> ### test/lib/jdk/test/lib/hprof/parser/HprofReader.java
> Ok.
>
> ### test/lib/jdk/test/lib/hprof/parser/Reader.java
>
> 93: is the created GzipRandomAccess instance closed somewhere?