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

Tue Apr 21 07:59:48 UTC 2020

Hi Ralf,

I've just started a closer review of your change, based on your current webrev. I'm not done yet but hope I'll find the time to finish this in the next few days.

However, as this patch seems in a quite good condition already and we're targeting JDK15, could you please start creating the required CSR to get it through the CSR process in parallel to the final review?

Thanks
Christoph

> -----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/HeapDumpCompressedTestEpsilo
> n.java
> Ok.
> 
> ###
> test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpCompressedTestShen
> andoah.java
> Ok.
> 
> ###
> test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpCompressedTestZ.jav
> a
> 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?