RFR: 8223438: add VirtualizationInformation JFR event

[Baesken, Matthias]

Hi Christoph,  the man page of  perfstat_partition_total   says :

Return Values

       Upon successful completion, the number of structures filled is returned. If unsuccessful, a value of -1 is returned and the errno global variable is set.

I call  with 1 structure  to  fill  , see ;

 392   perfstat_partition_total_t pinfo;
 393   rc = perfstat_partition_total(NULL, &pinfo, sizeof(perfstat_partition_total_t), 1);


So I think this is fine . Do you have any concerns ?

Best regards, Matthias



> -----Original Message-----
> From: Langer, Christoph
> Sent: Mittwoch, 15. Mai 2019 15:47
> To: Baesken, Matthias <matthias.baesken at sap.com>; Erik Gahlin
> <erik.gahlin at oracle.com>; 'hotspot-dev at openjdk.java.net' <hotspot-
> dev at openjdk.java.net>
> Subject: RE: RFR: 8223438: add VirtualizationInformation JFR event
> 
> Hi Matthias,
> 
> the change looks good to me.
> 
> In src/hotspot/cpu/ppc/vm_version_ppc.cpp, the correct SAP copyright
> header entry would be:
> * Copyright (c) 2012, 2019 SAP SE. All rights reserved.
> ...without comma after 2019. Should look like in
> src/hotspot/cpu/ppc/vm_version_ppc.hpp. Can you please fix that when
> pushing?
> 
> Also, in src/hotspot/cpu/ppc/vm_version_ppc.cpp, line 393, the call to
> perfstat_partition_total: Is 1 the success return code and if it returns 1, one
> can assume that PowerVM virtualization is available? If you can confirm that,
> it's fine ��
> 
> Best regards
> Christoph
> 
> > -----Original Message-----
> > From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf
> Of
> > Baesken, Matthias
> > Sent: Dienstag, 14. Mai 2019 10:02
> > To: Erik Gahlin <erik.gahlin at oracle.com>; 'hotspot-dev at openjdk.java.net'
> > <hotspot-dev at openjdk.java.net>
> > Subject: RE: RFR: 8223438: add VirtualizationInformation JFR event
> >
> > Thanks Erik , good to hear !
> > May I get a second review ?
> >
> > Best regards, Matthias
> >
> >
> > From: Erik Gahlin <erik.gahlin at oracle.com>
> > Sent: Freitag, 10. Mai 2019 15:15
> > To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
> > dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
> > Subject: Re: RFR: 8223438: add VirtualizationInformation JFR event
> >
> > Looks good.
> >
> > Erik
> >
> > Hello, here is   the next webrev  with  adjusted coding following your
> > suggestion :
> >
> >
> http://cr.openjdk.java.net/~mbaesken/webrevs/8223438.2/<http://cr.open
> > jdk.java.net/%7Embaesken/webrevs/8223438.2/>
> >
> >
> > Best regards, Matthias
> >
> >
> > From: Baesken, Matthias
> > Sent: Donnerstag, 9. Mai 2019 09:03
> > To: Erik Gahlin <erik.gahlin at oracle.com><mailto:erik.gahlin at oracle.com>;
> > 'hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>'
> > <hotspot-dev at openjdk.java.net><mailto:hotspot-
> dev at openjdk.java.net>
> > Subject: RE: RFR: 8223438: add VirtualizationInformation JFR event
> >
> > Hi   Erik,  the virtualization names  are a set of fix names ,  so I think we can
> do
> > what you suggest .
> >
> > I just "borrowed" a bit  from  the code above ...
> >
> > TRACE_REQUEST_FUNC(OSInformation) {
> >   ResourceMark rm;
> >   char* os_name = NEW_RESOURCE_ARRAY(char, 2048);
> >   JfrOSInterface::os_version(&os_name);
> >   EventOSInformation event;
> >   event.set_osVersion(os_name);
> >   event.commit();
> > }
> >
> >    ... where a ResourceMark + allocation is used (but  in jfrOSInterface I
> > wonder about the  second allocation in  string.stream.as_string , do we
> really
> > need two here ?
> >
> > int JfrOSInterface::JfrOSInterfaceImpl::os_version(char** os_version)
> const
> > {
> >   assert(os_version != NULL, "os_version pointer is NULL!");
> >   stringStream os_ver_info;
> >   os::print_os_info_brief(&os_ver_info);
> >   *os_version = os_ver_info.as_string();       <--- second alloc
> >
> > Best regards, Matthias
> >
> >
> >
> > From: Erik Gahlin
> <erik.gahlin at oracle.com<mailto:erik.gahlin at oracle.com>>
> > Sent: Mittwoch, 8. Mai 2019 18:55
> > To: Baesken, Matthias
> > <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>>;
> > 'hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>'
> > <hotspot-dev at openjdk.java.net<mailto:hotspot-
> dev at openjdk.java.net>>
> > Subject: Re: RFR: 8223438: add VirtualizationInformation JFR event
> >
> > Hello,
> >
> > Looks good, but do we really need to allocate the name every time an
> event
> > is emitted in jfrPeriodic.cpp?
> >
> > It will not matter much from a performance perspective, but the code
> would
> > look cleaner if we would have.
> >
> > EventVirtualizationInformation event;
> > event.set_name(JfrOSInterface::virtualization_name());
> > event.commit();
> >
> > Thanks
> > Erik
> > Hello, please review the following change .
> > It adds  a  simple VirtualizationInformation   event  to JFR .
> >
> > The addition  to JFR  has been previously  proposed by Erik  when  we
> > discussed the enhancement of hs_error file output  with virtualization
> > related information .
> >
> > Bug/webrev :
> >
> > https://bugs.openjdk.java.net/browse/JDK-8223438
> >
> >
> http://cr.openjdk.java.net/~mbaesken/webrevs/8223438.1/<http://cr.open
> > jdk.java.net/%7Embaesken/webrevs/8223438.1/>
> >
> >
> > Thanks, Matthias
> >