RFR: 8342818: Implement CPU Time Profiling for JFR

[Johannes Bechberger]

On Tue, 10 Sep 2024 13:26:26 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

> How is throttling handled? We don't want buffers to be filled up with sample events?

By setting the internal queue size. The user can throttle by setting the interval, any other option would lead to bias.

> Markus and I discussed the naming, and we are leaning against using "throttle" as the setting name. It is easier for users to understand if allocation, exception, socket read, etc., can share the same name for a rate like 500/s.

Just to clarify, do you mean that you're against using "rate" as a setting name?

I like the idea of overriding the "throttle" setting for different event types, which reduces the PR's complexity.

Let's see how the implementation goes.

> src/hotspot/share/jfr/metadata/metadata.xml line 939:
> 
>> 937:   </Event>
>> 938: 
>> 939:   <Event name="CPUTimeExecutionSample" category="Java Virtual Machine, Profiling" label="CPU Time Method Profiling Sample" description="Snapshot of a threads state from the CPU time sampler"
> 
> Maybe we should leave out "Execution" and only have CPUTimeSample?

Sounds like a good idea, because it combines both `ExecutionSample` and `NativeMethodSample`.

> src/jdk.jfr/share/classes/jdk/jfr/internal/util/ParsedCPUThrottle.java line 33:
> 
>> 31:  * A rate or fixed period, see {@link jdk.jfr.internal.Rate}
>> 32:  */
>> 33: public sealed interface ParsedCPUThrottle {
> 
> This seems complicated. Why isn't possible to keep the Rate class as is (no renaming) and then only create a TimespanRate class and do something like this:
> 
>     public record TimespanRate(double rate) {
>     
>         public static TimespanRate of(String text) {
>            Rate r = Rate.of(text);
>            if (r != null) {
>               double rate = r.amount() * ...
>               return new TimespanRate(rate)
>            } else {
>               long v = ValueParser.parseTimespanWithInfinity(text);
>               double rate = v * ...
>               return new TimespanRate(rate);
>           }
>       }
>     
>       public boolean isHigher(TimespanRate that) {
>           return rate() > that.rate();
>       }
>     }

I can try to implement. I thought it would be easier to comprehend this way.

> test/jdk/jdk/jfr/event/profiling/TestCPUTimeEventThrottling.java line 76:
> 
>> 74: 
>> 75:     private static EventCount countEvents(int timeMs, String rate) throws Exception {
>> 76:         Recording recording = new Recording();
> 
> Use try-with-resources.

Fixed

> test/jdk/jdk/jfr/event/profiling/TestFullStackTrace.java line 44:
> 
>> 42:  * @requires vm.hasJFR
>> 43:  * @library /test/lib
>> 44:  * @run main/othervm jdk.jfr.event.profiling.TestFullStackTrace wall-clock
> 
> Create separate tests for CPU time sampling events. It makes them easier to problem list and disable for specific platforms. If you want to share verification code, create  helper class.

I created two test classes and moved almost all code to `BaseTestFullStackTrace`.

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

PR Comment: https://git.openjdk.org/jdk/pull/20752#issuecomment-2340862057
PR Comment: https://git.openjdk.org/jdk/pull/20752#issuecomment-2416252945
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1752018869
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1806470258
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1812860292
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r1756672272