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

[David Holmes]

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
> 
>