RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump

classic Classic list List threaded Threaded
89 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Lin Zang-2
8257234 : Add gz option to SA jmap to write a gzipped heap dump

-------------

Commit messages:
 - 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Changes: https://git.openjdk.java.net/jdk/pull/1712/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257234
  Stats: 327 lines in 4 files changed: 292 ins; 2 del; 33 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1712.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1712/head:pull/1712

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v2]

Lin Zang-2
> 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  refine comments

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1712/files
  - new: https://git.openjdk.java.net/jdk/pull/1712/files/a915f067..909ee247

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=00-01

  Stats: 18 lines in 1 file changed: 1 ins; 3 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1712.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1712/head:pull/1712

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v3]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  delete unnecessary print

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1712/files
  - new: https://git.openjdk.java.net/jdk/pull/1712/files/909ee247..ef353ac3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1712.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1712/head:pull/1712

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v3]

Chris Plummer-2
On Thu, 10 Dec 2020 03:08:48 GMT, Lin Zang <[hidden email]> wrote:

>> 8257234 : Add gz option to SA jmap to write a gzipped heap dump
>
> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>
>   delete unnecessary print

Changes requested by cjplummer (Reviewer).

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line 1776:

> 1774:             }
> 1775:         },
> 1776:         new Command("dumpheap", "dumpheap [gz=<1-9>] [filename]", false) {

There is a lot of replicated code in this command handler. I'd suggest checking for "gz=" first, regardless of the token count. Produce an error if it is missing and the token count is 2. Otherwise process it if present. Then fetch the filename argument.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java line 62:

> 60:         System.out.println("                             \tif gz specified, the heap dump is written");
> 61:         System.out.println("                             \tin gzipped format using the given compression level");
> 62:         System.err.println("                             \t1 (recommended) is the fastest, 9 the strongest compression.");

You need to address the fact that we now have help text that is multiple sentences. I don't like that there are no periods in this case (except you added one). And using periods implies that you should also start the sentence with an upper case letter. And if you are to do all this for this one option, then it should be done for all of them.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java line 158:

> 156:                             }
> 157:                             dumpfile = keyValue[1];
> 158:                         } else if (keyValue[0].equals("gz")) {

I don't see "gz" being rejected if "-heap" or "-heap:format=x" are used. I think it needs to be.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java line 203:

> 201:                 hgw = new HeapHprofBinWriter(gzLevel);
> 202:             } else {
> 203:                 System.err.println("Illegal compress level: " + gzLevel);

Should be "compression" level.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java line 130:

> 128:         System.out.println("    --binaryheap            To dump java heap in hprof binary format.");
> 129:         System.out.println("    --dumpfile <name>       The name of the dump file.");
> 130:         System.out.println("    --gz <1-9>              The compress level for gzipped dump file.");

Should be "compression" level.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java line 316:

> 314:            Map.entry("histo", "-histo"),
> 315:            Map.entry("clstats", "-clstats"),
> 316:            Map.entry("finalizerinfo", "-finalizerinfo"));

Indentation seems to be 3 instead of 4. And I think it's actually suppose to be 8 in this case.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 525:

> 523:     private void fillInHeapRecordLength() throws IOException {
> 524:         // For compression, the length is written by CompressedSegmentOutputStream
> 525:         if (isCompression()) return;

I would expect to find logic in `CompressedSegmentOutputStream` that is similar to what is being skipped here in `fillInHeapRecordLength()`, but I don't. For example, I don't see a check for overflow that results in an exception like there is a few lines below here.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1325:

> 1323:      * it behaves same as BufferedOutputStream.
> 1324:      * */
> 1325:     private class CompressedSegmentOutputStream extends BufferedOutputStream {

Is there a reason why `CompressedSegmentOutputStream` can't just be called `SegmentOutputStream` and always be enabled? That would simplify a lot of logic that currently is different based on whether or not you are compressing the output.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v3]

Chris Plummer-2
On Mon, 4 Jan 2021 22:00:34 GMT, Chris Plummer <[hidden email]> wrote:

>> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   delete unnecessary print
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java line 62:
>
>> 60:         System.out.println("                             \tif gz specified, the heap dump is written");
>> 61:         System.out.println("                             \tin gzipped format using the given compression level");
>> 62:         System.err.println("                             \t1 (recommended) is the fastest, 9 the strongest compression.");
>
> You need to address the fact that we now have help text that is multiple sentences. I don't like that there are no periods in this case (except you added one). And using periods implies that you should also start the sentence with an upper case letter. And if you are to do all this for this one option, then it should be done for all of them.

Also, it looks like "-heap:format=x" is valid, and results in a dump mode of MODE_HEAP_GRAPH_GXL instead of MODE_HEAP_GRAPH_HPROF_BIN. I assume that MODE_HEAP_GRAPH_GXL and gz or not compatible. This should be made more clear in the help text. Possibly there should just be a 3rd "-heap" help entry, this one for "-help:format=x".

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v3]

Lin Zang-2
In reply to this post by Chris Plummer-2
On Mon, 4 Jan 2021 22:35:24 GMT, Chris Plummer <[hidden email]> wrote:

>> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   delete unnecessary print
>
> Changes requested by cjplummer (Reviewer).

Hi @plummercj ,
Thanks very much for reviewing!
Sorry that I got stuck on other work and didn't read review message timely.
I will go through your suggestions carefully and  try to fix all issues you mentioned ASAP.
Thanks and Happy New Year!
-Lin

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v4]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  refine segment heap dump

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1712/files
  - new: https://git.openjdk.java.net/jdk/pull/1712/files/ef353ac3..e7b53589

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=02-03

  Stats: 193 lines in 4 files changed: 47 ins; 50 del; 96 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1712.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1712/head:pull/1712

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v5]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Lin Zang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:

 - Merge branch 'master' into sa
 - refine segment heap dump
 - delete unnecessary print
 - refine comments
 - 8257234 : Add gz option to SA jmap to write a gzipped heap dump

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1712/files
  - new: https://git.openjdk.java.net/jdk/pull/1712/files/e7b53589..c720aca2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=03-04

  Stats: 99272 lines in 2659 files changed: 49749 ins; 30327 del; 19196 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1712.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1712/head:pull/1712

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v6]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  code refine

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1712/files
  - new: https://git.openjdk.java.net/jdk/pull/1712/files/c720aca2..b11434a8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=04-05

  Stats: 20 lines in 3 files changed: 1 ins; 3 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1712.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1712/head:pull/1712

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v3]

Lin Zang-2
In reply to this post by Lin Zang-2
On Fri, 8 Jan 2021 13:21:21 GMT, Lin Zang <[hidden email]> wrote:

>> Changes requested by cjplummer (Reviewer).
>
> Hi @plummercj ,
> Thanks very much for reviewing!
> Sorry that I got stuck on other work and didn't read review message timely.
> I will go through your suggestions carefully and  try to fix all issues you mentioned ASAP.
> Thanks and Happy New Year!
> -Lin

This PR has passed tier1 on my linux x86_64 box.
seems the fail of pre-submit test is cause by environment issue.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v6]

Yasumasa Suenaga-7
In reply to this post by Lin Zang-2
On Tue, 19 Jan 2021 05:43:12 GMT, Lin Zang <[hidden email]> wrote:

>> 8257234 : Add gz option to SA jmap to write a gzipped heap dump
>
> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>
>   code refine

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/JMap.java line 161:

> 159:                             dumpfile = keyValue[1];
> 160:                         } else if (keyValue[0].equals("gz")) {
> 161:                             String level = keyValue[1];

Same comment in above.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line 1788:

> 1786:                     String[] keyValue = option.split("=");
> 1787:                     if (keyValue[0].equals("gz")) {
> 1788:                         String level = keyValue[1];

If the user specified `gz` option incorrectly (e.g. `dumpheap gz` : without the level), `ArrayIndexOutOfBoundsException` will be thrown.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v7]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  fix issue of setting gz option with no value

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1712/files
  - new: https://git.openjdk.java.net/jdk/pull/1712/files/b11434a8..8dc60572

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=05-06

  Stats: 9 lines in 2 files changed: 9 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1712.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1712/head:pull/1712

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v7]

Chris Plummer-2
On Tue, 19 Jan 2021 12:39:12 GMT, Lin Zang <[hidden email]> wrote:

>> 8257234 : Add gz option to SA jmap to write a gzipped heap dump
>
> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>
>   fix issue of setting gz option with no value

I think you need to add some additional testing. You can use ClhsdbDumpheap.java and also BasicJMapTest.java. Besides testing with a compressed hprof file, also add a test for "gz=" with no compression value and with invalid compression value.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line 1792:

> 1790:                         return;
> 1791:                     }
> 1792:                     if (keyValue[0].equals("gz")) {

This code now assumes there will be a `gz` option specified. If there isn't, you will still execute this section of code, and get an NPE at this point.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 403:

> 401:         VM vm = VM.getVM();
> 402:
> 403:         // Check weather we should dump the heap as segments

Should be "whether"

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 475:

> 473:         if (!useSegmentedHeapDump) {
> 474:             // Fill in final length
> 475:             fillInHeapRecordLength();

It's unclear to me why this code is now conditional on not using a segmented heap dump.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1307:

> 1305:
> 1306:     /**
> 1307:      * The class implements a buffered output stream for segment data dump.

Should be "segmented data dump"

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1309:

> 1307:      * The class implements a buffered output stream for segment data dump.
> 1308:      * It is used inside HeapHprofBinWritter only for heap dump.
> 1309:      * Because the current implementation of segment heap dump needs to update

"segmented"

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1316:

> 1314:      * If the data to be written are larger than internal buffer, or the internal buffer
> 1315:      * is full, the internal buffer will be extend to a larger one.
> 1316:      * This class defines a switch to turn on/off the segment mode. if turned off,

"segmented". Also, uppercase "If".

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1317:

> 1315:      * is full, the internal buffer will be extend to a larger one.
> 1316:      * This class defines a switch to turn on/off the segment mode. if turned off,
> 1317:      * it behaves same as BufferedOutputStream.

"the same as"

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 517:

> 515:
> 516:     private void fillInHeapRecordLength() throws IOException {
> 517:         assert !useSegmentedHeapDump : "fillInHeapRecordLength is not supported for segment heap dump";

"segmented heap dump"

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1321:

> 1319:     private class SegmentedOutputStream extends BufferedOutputStream {
> 1320:         /**
> 1321:          * Creates a new buffered output stream to support segment heap dump data.

"segmented"

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1335:

> 1333:
> 1334:         /**
> 1335:          * Creates a new buffered output stream to support segment heap dump data.

"segmented"

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 1493:

> 1491:         // Segment support.
> 1492:         private boolean segmentMode;
> 1493:         private boolean allowSegmental;

"allowSegmented" would be a better name.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v6]

Chris Plummer-2
In reply to this post by Lin Zang-2
On Tue, 19 Jan 2021 05:43:12 GMT, Lin Zang <[hidden email]> wrote:

>> 8257234 : Add gz option to SA jmap to write a gzipped heap dump
>
> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>
>   code refine

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 490:

> 488:         hprofBufferedOut = null;
> 489:     }
> 490:

Can you explain what this check is for and why it is no longer needed?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v7]

Lin Zang-2
In reply to this post by Chris Plummer-2
On Tue, 19 Jan 2021 21:35:16 GMT, Chris Plummer <[hidden email]> wrote:

>> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   fix issue of setting gz option with no value
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 475:
>
>> 473:         if (!useSegmentedHeapDump) {
>> 474:             // Fill in final length
>> 475:             fillInHeapRecordLength();
>
> It's unclear to me why this code is now conditional on not using a segmented heap dump.

Hi Chris,
Thanks for review.  The original implementation of heap dump (when not using segments) is to treat all HPROF_HEAP_DUMP section as a whole segment and fill the size after the data are ready to be flush.
As specified at link: http://hg.openjdk.java.net/jdk6/jdk6/jdk/raw-file/tip/src/share/demo/jvmti/hprof/manual.html#mozTocId848088 .
so here if it is not segmented heap dump, it still need call fillInHeapRecordLength() to fill the size slot before data flush.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v6]

Lin Zang-2
In reply to this post by Chris Plummer-2
On Tue, 19 Jan 2021 21:38:28 GMT, Chris Plummer <[hidden email]> wrote:

>> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   code refine
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 490:
>
>> 488:         hprofBufferedOut = null;
>> 489:     }
>> 490:
>
> Can you explain what this check is for and why it is no longer needed?

The check here is to control the segment size. It checks whether the current segment is too large, if yes, it fills the segment size slot in fillInHeapRecordLength() and set  currentSegmentStart = 0, meaning to create a new segment for following data write.
And thanks to point it out, there should be a similar control logic in exitSegmentMode(), otherwise there is possibility of super huge segment.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v6]

Lin Zang-2
On Fri, 22 Jan 2021 07:06:17 GMT, Lin Zang <[hidden email]> wrote:

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 490:
>>
>>> 488:         hprofBufferedOut = null;
>>> 489:     }
>>> 490:
>>
>> Can you explain what this check is for and why it is no longer needed?
>
> The check here is to control the segment size. It checks whether the current segment is too large, if yes, it fills the segment size slot in fillInHeapRecordLength() and set  currentSegmentStart = 0, meaning to create a new segment for following data write.
> And thanks to point it out, there should be a similar control logic in exitSegmentMode(), otherwise there is possibility of super huge segment.

Just double checked the current implementation of segmentedOutputStream, it flush segmented data every time the exitSegmentMode() is called, so there can not be a super huge segment.
But the code is still required to control the size of segmented data to not be too small.
Thanks!

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v6]

Chris Plummer-2
On Fri, 22 Jan 2021 07:29:22 GMT, Lin Zang <[hidden email]> wrote:

>> The check here is to control the segment size. It checks whether the current segment is too large, if yes, it fills the segment size slot in fillInHeapRecordLength() and set  currentSegmentStart = 0, meaning to create a new segment for following data write.
>> And thanks to point it out, there should be a similar control logic in exitSegmentMode(), otherwise there is possibility of super huge segment.
>
> Just double checked the current implementation of segmentedOutputStream, it flush segmented data every time the exitSegmentMode() is called, so there can not be a super huge segment.
> But the code is still required to control the size of segmented data to not be too small.
> Thanks!

Ok

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v7]

Chris Plummer-2
In reply to this post by Lin Zang-2
On Fri, 22 Jan 2021 07:04:47 GMT, Lin Zang <[hidden email]> wrote:

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/HeapHprofBinWriter.java line 475:
>>
>>> 473:         if (!useSegmentedHeapDump) {
>>> 474:             // Fill in final length
>>> 475:             fillInHeapRecordLength();
>>
>> It's unclear to me why this code is now conditional on not using a segmented heap dump.
>
> Hi Chris,
> Thanks for review.  The original implementation of heap dump (when not using segments) is to treat all HPROF_HEAP_DUMP section as a whole segment and fill the size after the data are ready to be flush.
> As specified at link: http://hg.openjdk.java.net/jdk6/jdk6/jdk/raw-file/tip/src/share/demo/jvmti/hprof/manual.html#mozTocId848088 .
> so here if it is not segmented heap dump, it still need call fillInHeapRecordLength() to fill the size slot before data flush.

ok

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v8]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8257234 : Add gz option to SA jmap to write a gzipped heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  fix serveral issues and add test cases

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1712/files
  - new: https://git.openjdk.java.net/jdk/pull/1712/files/8dc60572..49a3a92c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1712&range=06-07

  Stats: 256 lines in 4 files changed: 190 ins; 14 del; 52 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1712.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1712/head:pull/1712

PR: https://git.openjdk.java.net/jdk/pull/1712
12345