RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Jun 3 13:14:40 UTC 2020
Hi Ralf,
I had a look at your change, webrev.3.
Thanks for contributing this!
Overall, a nicely engineered piece of work. Thus, my
comments are mostly minor details:
diagnosticCommand.hpp
ok.
diagnosticCommand.cpp:
l.510
I would be a bit more precise in the comment:
..."9 the slowest level with the best compression."
or maybe "strongest compression"?
l. 528
I would appreciate if you fixed the existing comment wrt. to the
language:
// Request a full GC before dumping the heap if _all is false.
// This helps reduce the amount of unreachable objects in the dump
heapDumper.hpp
ok.
heapDumper.cpp
Error messags is now recorded in _backend. ok.
Not overwriting file is moved to FileWriter, ok.
I like how you split the existing code with few
changes to distribute the work to the thread gang, nice!
l.1808
// Now we clear the global variables, so that a future dumper might run.
Is "might" correct? Isn't is "can"?
l.1819
// Write the file header - we always use 1.0.
You lost the ".2" from 1.0.2.
heapDumperCompression.hpp
Usually, in the include guards, only '/' are
replaced by '_'.
l.31
Extra whitespace before "implementation".
l.36
Initialized --> Initializes
Return --> Returns
it initialized --> initializes
l.119
works --> WriteWorks ... I had to think about this
a while to figure it's not a typo of 'work' but names
WriteWork instances in short. But the term is used throughout
the code, so maybe leave it as-is.
l.163
Remove "to".
l.165
returns the old --> commits the old ... or the like.
l.210
type-o maxiumum
heapDumperCompression.cpp
It's a bit confusing that the static variable
is called gzip_func (referring to a dedicated
function), while there is a method load_gzip_func
that loads any function from the gzip library.
What about gzip_zip_func for the variable?
l.113
What's the point of increasing needed_out_size
after the call? You increment the pointer?
l.125
add "of the":
good choice of the buffer sizes
CompressionBackend():
The check not to overwrite the inital, first error is in
set_error(). ok.
l.224
I think the comment should say "write the last remaining partially...."
l.400
I had one overall question, which I think is ansered here
at least partially:
As I understand, writing the dump now needs more buffer memory,
as there are several WriteWorks held at the same time.
Are they smaller than the buffer used before, so no additional
memory is needed, or is there a fallback if only a few can be
allocated? Is the fallback implemented here implicitly? Just
because if there is no memory for more works, the algorithm uses the
ones it could allocate, which might result in some idle
threads as there are less works than threads?
This makes it more flexible wrt. to available memory
than the implementation before, right?
l.441 indentation
l.458
I can't understand why this variable is named "left".
Is this past tense of to leave? Or do you mean the
left, filled, side of the buffer?
Another question.
The basic dumping is done sequential, right? The comression
is parallel. Is there a tradeoff in #of threads where
the compression is faster than writing?
zip_util.c
Looks good.
I appreciate the precise error message handling you are doing.
Could you please add comments that these functions are used
for heap dump compression?
HprofReader.java
ok.
Reader.java
Should you close in and in2 in case of error?
GzipRandomAccess.java
l.146
closes -> close
l.158 "the the"
This file nicely demonstrates how to read the zipped hprof.
Maybe you can add a hint in the JBS issue to this file?
HeapDumpCompressedTest.java
ok.
The other Tests:
Please merge them all into HeapDumpCompressedTest by using repeated
@test comments. You might not be aware this is
supported by jtreg. See test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NullPointerExceptionTest.java for an example.
It will run each @test block sperately and evaluate the @requires as expected.
Best regards,
Goetz.
-----Original Message-----
From: serviceability-dev <serviceability-dev-bounces at openjdk.java.net> On Behalf Of Schmelter, Ralf
Sent: Montag, 18. Mai 2020 09:23
To: Langer, Christoph <christoph.langer at sap.com>
Cc: serviceability-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net runtime <hotspot-runtime-dev at openjdk.java.net>
Subject: [CAUTION] RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
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 at sap.com>
Sent: Friday, 1 May 2020 18:46
To: Schmelter, Ralf <ralf.schmelter at sap.com>
Cc: serviceability-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net runtime <hotspot-runtime-dev at openjdk.java.net>; coleen.phillimore at 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 at oracle.com <coleen.phillimore at oracle.com>
> Sent: Montag, 20. April 2020 14:13
> To: Reingruber, Richard <richard.reingruber at sap.com>; 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, 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/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?
More information about the hotspot-runtime-dev
mailing list