Hi Ralf,
I finally managed to fully read through your change. Very nice piece of work.
I only found a few minor nits which would be nice if you could address them
before pushing. But no need for further webrev. Here we go:
workgroup.cpp
- update copyright year
L111: little spelling issue: forergound -> foreground
diagnosticCommand.cpp
L509: spelling recommneded -> recommended
L510: Initialization of default value ("1") is not necessary as current
implementation wouldn't allow the parameter -gz without value.
heapDumperCompression.hpp and heapDumperCompression.cpp:
License header says: Copyright (c) 2005, 2020, Oracle and/or its affiliates.
All rights reserved.
However, it's a net new file, so it should just be 2020,
Also, since this is new code, coming from SAP, you should credit SAP in the
copyright header (same way as you have done it in the test files).
test/lib/jdk/test/lib/hprof/parser/GzipRandomAccess.java:
L88: new ArrayList<> (diamond operator without type)
Thanks & Best regards
Christoph
> -----Original Message-----
> From: Schmelter, Ralf <ralf.schmel...@sap.com>
> Sent: Montag, 8. Juni 2020 11:38
> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Langer, Christoph
> <christoph.lan...@sap.com>
> Cc: serviceability-dev@openjdk.java.net; hotspot-runtime-
> d...@openjdk.java.net runtime <hotspot-runtime-...@openjdk.java.net>;
> David Holmes <david.hol...@oracle.com>; serguei.spit...@oracle.com; Ioi
> Lam <ioi....@oracle.com>
> Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap
> dump
>
> Hi Goetz,
>
> > What kind of tests did you run?
>
> The jdk submit repo, the JCK tests (apart from API) and the jtreg tests on
> Windows x86/64, MacOS X, linux on x86/64, ppcle, ppcbe, zarch and aarch64
> and on AIX.
>
> If there aren't any other concerns, I would like to commit this this change on
> Wednesday.
>
> Best regards,
> Ralf
>
> -----Original Message-----
> From: Lindenmaier, Goetz <goetz.lindenma...@sap.com>
> Sent: Friday, 5 June 2020 18:02
> To: Schmelter, Ralf <ralf.schmel...@sap.com>; Langer, Christoph
> <christoph.lan...@sap.com>
> Cc: serviceability-dev@openjdk.java.net; hotspot-runtime-
> d...@openjdk.java.net runtime <hotspot-runtime-...@openjdk.java.net>
> Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap
> dump
>
> Hi Ralf,
>
> Thanks for the quick reply and all the fixes.
> The changes to the workgroup are ok.
> Reviewed. (An incremental webrev would have helped 😊)
>
> What kind of tests did you run?
>
> > Yes, the buffer is now smaller (1M) versus the original (8M). You need
> > to be able to at least allocate one buffer or you get an error (this
> > is handled in the CompressionBackend ctor). You then allocate
> > additional buffers as needed (we want a new buffer, but there is no
> > free one), until we have a buffer for every worker thread or until
> > the allocation of the buffer failed. In this case some threads will
> > be idle, since we cannot have a buffer for each thread.
> Ok, that's what I thought. Thanks for the explanation.
>
> > >Â 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?
> > Yes. The compression and writing is done parallel. Depeding on
> > the compression level and the speed of your harddrive, not all
> > threads will be active all the time. But since we reuse the GC threads
> > this should not matter. And the relative poor performance of
> > deflate() ensures that at least 5 to 10 threads will probably always
> > be active ;)
> Ok, thanks.
>
> Best regards,
> Goetz.
