On Fri, 29 Jan 2021 05:39:59 GMT, Lin Zang <lz...@openjdk.org> wrote:
>> 8257234 : Add gz option to SA jmap to write a gzipped heap dump
>
> Lin Zang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - fix an issue of double printing error message.
> - fix jcmd jmap issue and add test in BascJMapTest and code refine
> > I have found that there is a bug in jcmd jmap, it could accept the argument
> > "gz=" with no value specified.
> > so jmap -dump:file=a.hprof,gz= could generate dump file successfully.
>
> After re-think of it, the jcmd jmap issue mentioned here seems not quite
> related with this PR, so maybe a standalone one is better? What do you think?
That's probably a good idea since this PR is for SA changes, and it's probably
best not to mix it with attach api changes.
test/jdk/sun/tools/jmap/BasicJMapTest.java line 121:
> 119: testDumpAll();
> 120: testDumpCompressed();
> 121: testDumpIllegalCompressedArgs();
Thanks for updating this test to better check the args, but it looks like I
pointed you to the wrong test. This is testing the pmap tool, which already had
the gz option. I actually meant for you to add testing for the gz option
support you added to "jhsdb jmap --binaryheap" , which is what you added to
SALauncher.java. The testing for "jhsdb jmap --binaryheap" is done in
test/jdk/sun/tools/jhsdb/HeapDumpTest.java.
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 compression
> level for gzipped dump file.");
`--dumpfile` and `--gz` can only be used with `--binaryheap`. That should be
made clear in the help text.
I just noticed that `jhsdb pmap` does not support the equivalent of `format=x`
that the jcmd version does. However, the support is actually there in the SA
JMap.java. It's confusing because both `jhsdb jmap` and the clhsdb `dumpheap`
command use the JMap class, but neither support passing `format=x` to it. In
order to get SA to dump with `format=x`, you would need to launch
`sun/jvm/hotspot/tools/JMap` directly and pass in `-heap:format=x`. I'm not
suggesting you fix that, but just something for us to be aware of.
One question though. Have you tried running JMap directly to make sure the help
output looks right. You've made fixes to it in this webrev, but since there are
so many different "jmap" commands (5), and each with their own help, I've lost
sight of whether or not this is even tested. I don't believe it is, but I
wouldn't worry about that. However, since made changes to the help output, it
would be good to at least check it out.
And if you are wonder what the 5 `jmap` commands are:
- SA's `jhsdb jmap --binaryheap` (which uses JMap.java with `-heap:format=b`)
- SA's clhsdb `dumpheap` command (which also uses JMap.java with
`-heap:format=b`)
- executing SA's `sun/jvm/hotspot/tools/JMap` class directly
- `jmap -dump` command (Uses Attach API's `heapdump` command, which uses
hotspot `HeapDump` class)
- `GC.heap_dump` dcmd, which also uses the hotspot `HeapDumper` class
So really there are two implementations of heap dumping, one in the VM and one
in SA, but a total of 5 ways to get to them. I'm just starting to get my head
wrapped around all this myself. We've created a very confusing situation for
us, and unfortunately I think some of the changes that got us here was driven
by a goal to simply things.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1712
