[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
From: "Schmelter, Ralf" <ralf.schmelter () sap ! com>
Date: 2020-05-18 7:22:54
Message-ID: AM6PR0202MB33521999AE326D5F94F9DD7E9FB80 () AM6PR0202MB3352 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]
Hi Christoph,
I've updated the webrev: \
http://cr.openjdk.java.net/~rschmelter/webrevs/8237354/webrev.3/
The significant changes are moving most of the new compression code to its own file, \
changing to use a single option (see CSR) called -gz with a mandatory compression \
level and to load the zlib only once (analog to the new class loader code). \
Additionally I've removed some long lines.
Best regards,
Ralf
-----Original Message-----
From: Langer, Christoph <christoph.langer@sap.com>
Sent: Friday, 1 May 2020 18:46
To: Schmelter, Ralf <ralf.schmelter@sap.com>
Cc: serviceability-dev@openjdk.java.net; hotspot-runtime-dev@openjdk.java.net runtime \
<hotspot-runtime-dev@openjdk.java.net>; coleen.phillimore@oracle.com
Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
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.phillimore@oracle.com <coleen.phillimore@oracle.com>
> Sent: Montag, 20. April 2020 14:13
> To: Reingruber, Richard <richard.reingruber@sap.com>; 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, 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@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