[RFR]8215622: Add dump to file support for jmap histo

[Hohensee, Paul]

I agree, let’s leave the dump option alone for now. Changing “and” to “or” is just a slight grammar correctness fix.

Thanks,

Paul

From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Monday, February 11, 2019 at 6:33 PM
To: 臧琳 <zanglin5 at jd.com>, "Joseph D. Darcy" <joe.darcy at oracle.com>, "Hohensee, Paul" <hohensee at amazon.com>, David Holmes <david.holmes at oracle.com>, JC Beyler <jcbeyler at google.com>
Cc: "serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net>
Subject: Re: [RFR]8215622: Add dump to file support for jmap histo

Dear Lin,


On 2/11/19 17:49, 臧琳 wrote:

Dear All,

     Thanks again for the comments.

     one question from me is that "jmap -dump" must accept an argument indicates location of the heap dump file. if there is no such argument, it fails. (it is meaningless to output the binary directly to stdout/stderr for me.)

Does the option "format=b" tells us that the dump output can be in text format as well?
The help tells nothing about what is the default format.
I'd conclude that the default has to be text format if the option "format=b" is missed.

Now, I'm looking at the option processing in the dump() method and see that option "format=b" is not really processed but just filtered out.
It means that you are right, the only supported dumping format is binary.

In this situation, I withdraw my suggestion to allow empty <dump-options>.
At least, you do not need to care about it.
However, there is still the suggestion from Paul to replace "and" with "or" in both dump-options and histo-options.



     so adding "jmap -dump" actually just output error message like "No dump file specified", rather than print the usage of JMap as implemented now.

     And this is also why there is no "-dump" or "-dump:" test in BasicJMapTest.java.

I leave it to other people to decide what is better here.



     Another option is to add default file generated at canonical path for -dump. then I can add the unit test.

     which one do you think is more reasonable?

It looks like a good idea.
But I'm not sure about it yet.
Let's see what others say.



     Moreover, one minor thing is that BasicJMapTest right now has test for "jmap -histo:" , not for "jmap -histo", so I think I need to add test for that. is this OK?

Yes.


Thanks,
Serguei






Cheers,

Lin

________________________________________

From: Joseph D. Darcy <joe.darcy at oracle.com><mailto:joe.darcy at oracle.com>

Sent: Tuesday, February 12, 2019 6:22:59 AM

To: Hohensee, Paul; serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>; 臧琳; David Holmes; JC Beyler

Cc: serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>

Subject: Re: [RFR]8215622: Add dump to file support for jmap histo



On 2/11/2019 2:20 PM, Hohensee, Paul wrote:

The CSR was just closed, so we're stuck with doing another one later.

Before a changeset is pushed, its CSR can be amended (Withdraw or back

to draft, then refinalize after editing.)



HTH



-Joe


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190212/38d9843f/attachment.html>