RFR: 8223438: add VirtualizationInformation JFR event

[Langer, Christoph]

Hi Matthias,

thanks for the explanation. Looks good to me.

Best regards
Christoph

> -----Original Message-----
> From: Baesken, Matthias
> Sent: Mittwoch, 15. Mai 2019 17:40
> To: Langer, Christoph <christoph.langer 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 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
> > >