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

[Thomas Stüfe]

Thanks a lot, David!

On Mon, Jun 3, 2019 at 8:27 AM David Holmes <david.holmes at oracle.com> wrote:

> Hi Thomas,
>
> Thanks for clarifying a few things. The updates look fine to me.
>
> For the CSR I suggest moving it to Finalized.
>
> Thanks,
> David
>
> On 3/06/2019 4:08 pm, Thomas Stüfe wrote:
> > 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
> > <mailto: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
> >
> >
>