On Wed, 4 Jun 2025 15:48:31 GMT, Johannes Bechberger <jbechber...@openjdk.org> wrote:
>> This is the code for the [JEP 509: CPU Time based profiling for >> JFR](https://openjdk.org/jeps/509). >> >> Currently tested using [this test >> suite](https://github.com/parttimenerd/basic-profiler-tests). This runs >> profiles the [Renaissance](https://renaissance.dev/) benchmark with >> - ... different heap sizes >> - ... different GCs >> - ... different samplers (the standard JFR and the new CPU Time Sampler and >> both) >> - ... different JFR recording durations >> - ... different chunk-sizes > > Johannes Bechberger has updated the pull request incrementally with one > additional commit since the last revision: > > Improve I admit I am leaning pretty hard on JFR folks expertise here. I agree this code passes the bar for experimental feature: it does not seem to affect non-JFR paths, does not seem to interact with JFR in obviously incorrect manner, and in itself looks more or less sensible. Please file the follow-ups to figure out the memory ordering story in `JfrCPUTimeTraceQueue`. src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 640: > 638: > 639: VM_CPUTimeSamplerThreadInitializer(JfrCPUSamplerThread* sampler) : > _sampler(sampler) { > 640: } Suggestion: private: JfrCPUSamplerThread* const _sampler; public: VM_CPUTimeSamplerThreadInitializer(JfrCPUSamplerThread* sampler) : _sampler(sampler) {} src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 671: > 669: > 670: VM_CPUTimeSamplerThreadTerminator(JfrCPUSamplerThread* sampler) : > _sampler(sampler) { > 671: } Suggestion: private: JfrCPUSamplerThread* const _sampler; public: VM_CPUTimeSamplerThreadTerminator(JfrCPUSamplerThread* sampler) : _sampler(sampler) {} src/hotspot/share/runtime/vmOperation.hpp line 119: > 117: template(RendezvousGCThreads) \ > 118: template(CPUTimeSamplerThreadInitializer) \ > 119: template(CPUTimeSamplerThreadTerminator) \ I think these better be prefixed with `JFR`. E.g.: `JFRInitializeCPUTimeSampler` / `JFRTerminateCPUTimeSampler`? ------------- Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25302#pullrequestreview-2897511962 PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2127078325 PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2127079549 PR Review Comment: https://git.openjdk.org/jdk/pull/25302#discussion_r2127090498
