On Fri, 25 Apr 2025 10:38:39 GMT, Casper Norrbin <cnorr...@openjdk.org> wrote:
> Hi everyone,
>
> This change removes the legacy `PerfData` sampling mechanism implemented
> through the `StatSampler` — an always-on periodic task that runs every 50ms
> my default. The sampling feature was originally introduced to collect
> performance counters and timestamps, but has since seen very little use.
>
> For G1/ZGC, the only sampled value is a timestamp (`sun.os.hrt.ticks`). For
> Serial/Parallel, it also samples some heap space counters, but these are
> already updated after each GC cycle, making the sampling redundant. With
> sampling removed, the `PerfDataSamplingInterval` flag becomes obsoleted, as
> it no longer serves any purpose.
>
> The only thing relying on the sampled timestamps is `jstat`: running `jstat
> -t` prints an extra column with the time since VM start. To preserve this
> funcitonality, we can calculate the timestamps as an offset from the already
> existing `sun.rt.createVmBeginTime` instead.
There are still a few matches for "hrt.ticks"; don't know if they should be
removed (in this PR or a followup).
src/hotspot/share/runtime/arguments.cpp line 538:
> 536: { "ZGenerational", JDK_Version::jdk(23),
> JDK_Version::jdk(24), JDK_Version::undefined() },
> 537: { "ZMarkStackSpaceLimit", JDK_Version::undefined(),
> JDK_Version::jdk(25), JDK_Version::undefined() },
> 538: { "PerfDataSamplingInterval", JDK_Version::undefined(),
> JDK_Version::jdk(25), JDK_Version::undefined() },
Since the CSR says it will be removed in 26, the final arg should reflect that,
IMO.
src/hotspot/share/runtime/perfData.cpp line 419:
> 417: */
> 418: void PerfDataManager::assert_system_property(const char* name, const
> char* value, TRAPS) {
> 419: #ifdef ASSERT
The indentation seems odd. (I recall `#ifdef` itself should not indented.)
src/hotspot/share/runtime/perfData.cpp line 455:
> 453: assert(value != nullptr, "property name should be have a value: %s",
> name);
> 454: assert_system_property(name, value, CHECK);
> 455: if (value != nullptr) {
Why checking null again? Didn't we just asserted that 2 lines above?
src/hotspot/share/runtime/threads.cpp line 852:
> 850: #endif // INCLUDE_MANAGEMENT
> 851:
> 852: PerfDataManager::create_misc_perfdata();
Should this be guarded by `UsePerfData`?
-------------
PR Review: https://git.openjdk.org/jdk/pull/24872#pullrequestreview-2795935371
PR Review Comment: https://git.openjdk.org/jdk/pull/24872#discussion_r2061262368
PR Review Comment: https://git.openjdk.org/jdk/pull/24872#discussion_r2061263070
PR Review Comment: https://git.openjdk.org/jdk/pull/24872#discussion_r2061263372
PR Review Comment: https://git.openjdk.org/jdk/pull/24872#discussion_r2061264533
