Hi Ralf, while I'm reviewing your change I think extracting the compression coding to an own file would be a good idea. Maybe you could name it heapDumpCompression.cpp?
When looking at the webrev I also figured that there are some very long lines (beyond 90 chars or so). Maybe you could have a look if you could shorten some of them and break a few of these long lines? More detailed review to follow. Best regards Christoph > -----Original Message----- > From: coleen.phillim...@oracle.com <coleen.phillim...@oracle.com> > Sent: Montag, 20. April 2020 14:13 > To: Reingruber, Richard <richard.reingru...@sap.com>; Schmelter, Ralf > <ralf.schmel...@sap.com>; Ioi Lam <ioi....@oracle.com>; Langer, Christoph > <christoph.lan...@sap.com>; Yasumasa Suenaga > <suen...@oss.nttdata.com>; serguei.spit...@oracle.com; hotspot- > runtime-...@openjdk.java.net runtime <hotspot-runtime- > d...@openjdk.java.net> > Cc: serviceability-dev@openjdk.java.net > Subject: Re: RFR(L) 8237354: Add option to jcmd to write a gzipped heap > dump > > 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.schmel...@sap.com> > > Sent: Montag, 20. April 2020 10:14 > > To: Reingruber, Richard <richard.reingru...@sap.com>; Ioi Lam > <ioi....@oracle.com>; Langer, Christoph <christoph.lan...@sap.com>; > Yasumasa Suenaga <suen...@oss.nttdata.com>; > serguei.spit...@oracle.com; hotspot-runtime-...@openjdk.java.net > runtime <hotspot-runtime-...@openjdk.java.net> > > Cc: serviceability-dev@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.reingru...@sap.com> > > Sent: Tuesday, 14 April 2020 10:30 > > To: Schmelter, Ralf <ralf.schmel...@sap.com>; Ioi Lam > <ioi....@oracle.com>; Langer, Christoph <christoph.lan...@sap.com>; > Yasumasa Suenaga <suen...@oss.nttdata.com>; > serguei.spit...@oracle.com; hotspot-runtime-...@openjdk.java.net > runtime <hotspot-runtime-...@openjdk.java.net> > > Cc: serviceability-dev@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?