RFR(s): 8224600: Provide VM.events command

[Thomas Stüfe]

Hi David,

thanks for reviewing!

New webrev below and comments inline.

On Thu, May 30, 2019 at 8:48 AM David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> On 24/05/2019 5:51 am, Thomas Stüfe wrote:
> > Hi all,
> >
> > May I please have reviews for the following addition to jcmd.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8224600
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8224601
> > webrev:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/vm.events/webrev.00/webrev/index.html
>
> This seems quite reasonable but I have a couple of queries/comments:
>
> Where do the short-names of the logs get used? Is that what you ask for
> via the Dcmd?


I added the short names (now renamed to "handle") because selecting by the
event log full name was cumbersome. The full name is more of a headline
consisting of multiple words, e.g. "Deoptimization events".


> How does the user know what logs are available?
>

I print a list of valid names out if the user specifies an event log which
does not exist. See Events::print_one().

I would have liked to print this list as part of the command help; however,
that would have made the patch larger, added code duplication and made it
much more complicated since the help command (HelpDCmd) is not really
flexible and cannot deal easily with dynamic content. Especially not
content which is order-of-initialization-dependend (diagnostic command
initialization vs event log initialization).


>
>
> // Print log names (for help output of VM.events
>
> This comment appears in a few places but is missing the final ).
>
> The test should have a single copyright year of 2019.
>
>
Fixed.


> The test should be checking all the variations to ensure if you ask for
> exceptions you only get exceptions etc.
>
>
Okay, I did add a test testing the selection feature. Since all I test is
for the log event *headers* to appear - regardless of whether or not they
are empty - this is okay. Any test relying on actual event log content
would be way more difficult to do.

Please note that I only tested the selection feature to work, but I see
little value in testing any permutation of the "log" option. Would just
make the test larger and more brittle and burn test machine time without
any real value.

Is the test guaranteed to see events of all types? In particular class
> unloading may not occur in a short run (and only anonymous classes would
> get unloaded unless custom loaders are involved).
>
>
See above. Only test for the headers to appear. They also appear when the
log is empty (e.g.:

<quote>
Deoptimization events (0 events):
No events

Classes unloaded (0 events):
No events

Classes redefined (0 events):
No events

Internal exceptions (0 events):
No events

Events (20 events):
Event: 0,063 Executing VM operation: RevokeBias done
Event: 0,071 Executing VM operation: HandshakeAllThreads
Event: 0,071 Executing VM operation: HandshakeAllThreads done
...
</quote>

New webrev:
delta:
http://cr.openjdk.java.net/~stuefe/webrevs/vm.events/webrev_delta.01/webrev/
full: http://cr.openjdk.java.net/~stuefe/webrevs/vm.events/webrev.01/webrev/

I renamed "short name" to "handle" and rewrote a number of comments to make
the code clearer.

Do you know when I can expect the CSR to be approved?

Thanks & Cheers, Thomas


>