On Tue, 7 Nov 2023 01:06:12 GMT, Man Cao <m...@openjdk.org> wrote: > I think it looks great. It is mainly refactoring that consolidates the > declarations/definitions of the hsperf counters in to a single file. Would it > be better to name that class `CPUTimeCounters`, so we could move > `sun.threads.cpu_time.vm` and `sun.threads.cpu_time.conc_dedup`, and future > JIT thread CPU counters to that class? > Yes, mainly refactoring and I was thinking along the same lines, but since this patch was just for GC and we had `CollectorCounters` already I went with this. I think calling the class `CPUTimeCounters` would be good and place it outside GC makes sense if we plan to include even more CPU time counters.
Another name that we could improve is `CPUTimeGroups` and maybe also the enum name `Name`, they are ok, but we might come up with something better. > Then we could also change the constructor of `ThreadTotalCPUTimeClosure` to > `ThreadTotalCPUTimeClosure(CPUTimeCounters* counters, CPUTimeGroups::Name > name)`, then it could set `_update_gc_counters` based on `name`. I was looking at this too, but had to restructure the code more to avoid circular deps. If we create the general `CPUTImeCounters` we could move this closure to that file and then things would fit better I believe. So I like your proposals. ------------- PR Comment: https://git.openjdk.org/jdk/pull/15082#issuecomment-1798170871
