[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
From:       Man Cao <manc () openjdk ! org>
Date:       2023-11-30 22:03:28
Message-ID: clkWdZZLE8vd-cTCS-R275cr20Gj_bsYIw3pNTWpvpI=.3778998f-a7b0-4bb4-a88c-590bc39464a4 () github ! com
[Download RAW message or body]

On Thu, 30 Nov 2023 09:41:55 GMT, Stefan Johansson <sjohanss@openjdk.org> wrote:

> > Jonathan Joo has updated the pull request incrementally with one additional \
> > commit since the last revision: 
> > Add missing include
> 
> src/hotspot/share/runtime/cpuTimeCounters.hpp line 59:
> 
> > 57:   NONCOPYABLE(CPUTimeCounters);
> > 58: 
> > 59:   static CPUTimeCounters* _instance;
> 
> I would prefer if we made the whole class static and got rid of the instance and \
> just made the `_cpu_time_counters` array static. The only drawback I/we (discussed \
> this with Albert as well) can see is that the memory for the array would not be \
> accounted in NMT, but this array will always be very small so should not be a big \
> problem.  
> Do you see any other concerns?

I thought it is typically preferred to initialize a singleton object on the heap, \
rather than using several static variables. It is easier to control the \
initialization order and timing of an on-heap singleton object than statics.

Moreover, for this class, `initialize()` could also check `if (UsePerfData)`, and \
only create the singleton object under `UsePerfData`. This could save some memory \
when `UsePerfData` is false.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1411317828


[prev in list] [next in list] [prev in thread] [next in thread]