RFR: 8221535: add steal tick related information to hs_error file [linux]

[Baesken, Matthias]

Hi David ,   thanks  for  your comments .

New webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8221535.2/

moved  os::CPUPerfTicks    to   os::Linux::CPUPerfTicks
removed DEBUG_LINUX_PROC_STAT  from os_linux.cpp
addressed the minor  comments


Best regards, Matthias


> -----Original Message-----
> From: David Holmes <david.holmes at oracle.com>
> Sent: Freitag, 29. März 2019 06:07
> To: Baesken, Matthias <matthias.baesken at sap.com>; Thomas Stüfe
> <thomas.stuefe at gmail.com>
> Cc: hotspot-dev at openjdk.java.net
> Subject: Re: RFR: 8221535: add steal tick related information to hs_error file
> [linux]
> 
> Hi Matthias,
> 
> This generally seems okay.
> 
> On 28/03/2019 2:50 am, Baesken, Matthias wrote:
> > Hello, here is a second webrev :
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8221535.1/
> >
> > I  followed  Davids idea  to  use  the  os_perf coding .
> > For reusing  I  moved  os::Linux::get_tick_information    (former name
> get_total_ticks  , but it is now not only total  but also steal  )   into  os_linux .
> > The CPUPerfTicks   struct has been  enhanced   by    steal and  a boolean flag
> has_steal_ticks  .
> >
> >    28 struct CPUPerfTicks {
> >    29   uint64_t used;
> >    30   uint64_t usedKernel;
> >    31   uint64_t total;
> >    32   uint64_t steal;
> >    33   bool     has_steal_ticks;
> >    34 };
> 
> Okay, but one minor nit: why put struct CPUPerfTicks in the os::
> namespace with the code that uses it in the os::Linux namespace?
> 
> A few other minor comments:
> 
> src/hotspot/os/linux/os_linux.cpp
> 
> +   if (-1 == which_logical_cpu) {
> 
> hotspot style puts the variable first in a comparison against a constant.
> 
> + #ifdef DEBUG_LINUX_PROC_STAT
> 
> This should either just be DEBUG code or else deleted. The vm_printfs
> would ideally be logging statements if that is possible.
> 
> +   if (n > expected_tickinfo_count+3) {
> 
> Spaces around + please.
> 
> +       st->print_cr("Steal ticks from vm start: %llu", (long long
> unsigned) steal_ticks_difference);
> 
> %llu should be UINT64_FORMAT.
> The cast should not be needed.
> 
> Thanks,
> David
> -----
> 
> >
> > I found  the  "big API"   idea  a bit too much  for  just 2  use cases  .
> >
> >
> > Best regards , Matthias
> >