Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Sat, 2 Dec 2023 01:22:33 GMT, Jonathan Joo wrote: >> I still think that a total counter is useful and I'd appreciate if you can >> keep it. To second what @caoman said before, it is GC agnostic, easy to use >> even for non GC experts and future proof with regards to implementation >> changes in the GCs. Please keep it. > > Put the closure in a scope, I think that should address the concern. Yes, scoping it will work. - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1413591437
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Fri, 1 Dec 2023 14:42:22 GMT, Albert Mingkun Yang wrote: >> I would say it depends on the use-case and here when switching to use static >> functions to use the instance it felt more like an all-static class. I agree >> that it would be nice to avoid the additional memory usage if `UsePerfData` >> is `false` so I'm ok with keeping the instance if we add that. > >> It is easier to control the initialization order and timing of an on-heap >> singleton object than statics. > > It's generally true, but the init of CPUTimeCounters is not sensitive to > ordering. > >> This could save some memory when UsePerfData is false. > > True, but the mem savings will be marginal at most, `PerfCounter* > _cpu_time_counters[static_cast(CPUTimeGroups::CPUTimeType::COUNT)];` > will be 8 * ~12 = ~96 bytes (including future cpu-time-type enums). > > (I don't have a strong preference here; however, I'd like to make the pros > and cons explicit.) Added the check in the initializer - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1412749097
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Fri, 1 Dec 2023 16:19:49 GMT, Volker Simonis wrote: >> src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 905: >> >>> 903: gc_threads_do(); >>> 904: >>> 905: CPUTimeCounters::publish_gc_total_cpu_time(); >> >> As I suggested in the other comment, maybe we should not keep the total >> counter, but if we do we need to make sure the destructor of the closure is >> run before the call to `publish_gc_total_cpu_time()`. Otherwise we will >> publish a not yet updated value. > > I still think that a total counter is useful and I'd appreciate if you can > keep it. To second what @caoman said before, it is GC agnostic, easy to use > even for non GC experts and future proof with regards to implementation > changes in the GCs. Please keep it. Put the closure in a scope, I think that should address the concern. - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1412694432
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Thu, 30 Nov 2023 09:46:03 GMT, Stefan Johansson wrote: >> Jonathan Joo has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add missing include > > src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 905: > >> 903: gc_threads_do(); >> 904: >> 905: CPUTimeCounters::publish_gc_total_cpu_time(); > > As I suggested in the other comment, maybe we should not keep the total > counter, but if we do we need to make sure the destructor of the closure is > run before the call to `publish_gc_total_cpu_time()`. Otherwise we will > publish a not yet updated value. I still think that a total counter is useful and I'd appreciate if you can keep it. To second what @caoman said before, it is GC agnostic, easy to use even for non GC experts and future proof with regards to implementation changes in the GCs. Please keep it. - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1412322098
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Fri, 1 Dec 2023 09:58:16 GMT, Stefan Johansson wrote: >> 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. > > I would say it depends on the use-case and here when switching to use static > functions to use the instance it felt more like an all-static class. I agree > that it would be nice to avoid the additional memory usage if `UsePerfData` > is `false` so I'm ok with keeping the instance if we add that. > It is easier to control the initialization order and timing of an on-heap > singleton object than statics. It's generally true, but the init of CPUTimeCounters is not sensitive to ordering. > This could save some memory when UsePerfData is false. True, but the mem savings will be marginal at most, `PerfCounter* _cpu_time_counters[static_cast(CPUTimeGroups::CPUTimeType::COUNT)];` will be 8 * ~12 = ~96 bytes (including future cpu-time-type enums). (I don't have a strong preference here; however, I'd like to make the pros and cons explicit.) - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1412193296
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Thu, 30 Nov 2023 21:59:41 GMT, Man Cao wrote: >> 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. I would say it depends on the use-case and here when switching to use static functions to use the instance it felt more like an all-static class. I agree that it would be nice to avoid the additional memory usage if `UsePerfData` is `false` so I'm ok with keeping the instance if we add that. - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1411872139
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Thu, 30 Nov 2023 09:41:55 GMT, Stefan Johansson 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
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Thu, 30 Nov 2023 02:45:45 GMT, Jonathan Joo wrote: >> 8315149: Add hsperf counters for CPU time of internal GC threads > > Jonathan Joo has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing include src/hotspot/share/runtime/cpuTimeCounters.cpp line 47: > 45: return "conc_dedup"; > 46: default: > 47: ShouldNotReachHere(); My IDE complained and I guess depending on warning level we might need a return here. Suggestion: ShouldNotReachHere(); return ""; - PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1410454940
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
On Thu, 30 Nov 2023 02:45:45 GMT, Jonathan Joo wrote: >> 8315149: Add hsperf counters for CPU time of internal GC threads > > Jonathan Joo has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing include Few more comments after the latest changes. src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 905: > 903: gc_threads_do(); > 904: > 905: CPUTimeCounters::publish_gc_total_cpu_time(); As I suggested in the other comment, maybe we should not keep the total counter, but if we do we need to make sure the destructor of the closure is run before the call to `publish_gc_total_cpu_time()`. Otherwise we will publish a not yet updated value. 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? - Changes requested by sjohanss (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15082#pullrequestreview-1757031262 PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1410415366 PR Review Comment: https://git.openjdk.org/jdk/pull/15082#discussion_r1410410265
Re: RFR: 8315149: Add hsperf counters for CPU time of internal GC threads [v53]
> 8315149: Add hsperf counters for CPU time of internal GC threads Jonathan Joo has updated the pull request incrementally with one additional commit since the last revision: Add missing include - Changes: - all: https://git.openjdk.org/jdk/pull/15082/files - new: https://git.openjdk.org/jdk/pull/15082/files/e6726ab6..7e4cdcd3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15082=52 - incr: https://webrevs.openjdk.org/?repo=jdk=15082=51-52 Stats: 2 lines in 2 files changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15082/head:pull/15082 PR: https://git.openjdk.org/jdk/pull/15082
