[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
From: "Langer, Christoph" <christoph.langer () sap ! com>
Date: 2020-04-21 7:59:48
Message-ID: AM0PR02MB571462808F4485FF3D407F4B8AD50 () AM0PR02MB5714 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]
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@sap.com>
> Sent: Montag, 20. April 2020 10:14
> To: Reingruber, Richard <richard.reingruber@sap.com>; Ioi Lam
> <ioi.lam@oracle.com>; Langer, Christoph <christoph.langer@sap.com>;
> Yasumasa Suenaga <suenaga@oss.nttdata.com>;
> serguei.spitsyn@oracle.com; hotspot-runtime-dev@openjdk.java.net
> runtime <hotspot-runtime-dev@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.reingruber@sap.com>
> Sent: Tuesday, 14 April 2020 10:30
> To: Schmelter, Ralf <ralf.schmelter@sap.com>; Ioi Lam
> <ioi.lam@oracle.com>; Langer, Christoph <christoph.langer@sap.com>;
> Yasumasa Suenaga <suenaga@oss.nttdata.com>;
> serguei.spitsyn@oracle.com; hotspot-runtime-dev@openjdk.java.net
> runtime <hotspot-runtime-dev@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?
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic