RFR: 8342818: Implement CPU Time Profiling for JFR

[Erik Gahlin]

On Fri, 27 Sep 2024 17:04:29 GMT, Johannes Bechberger <jbechberger at openjdk.org> wrote:

> The semantics are different, as `maxRate` is just upper bounding, while `throttle` is used to sub-sample (not using the throttle code under the hood), this is why I use a new name. Else I would have to check all over JFR which meaning `throttle` has for every event.

The throttle setting sets an upper bound, i.e 100 events / second, and then drops event.

Is the difference that you don't drop events? Otherwise, they seem similar.

Period on the other hand seems a bit off as this is specified in cpu time (i assume) and not wall-clock time. It could take seconds before an event with period 20 ms is emitted.

The period we have today for the ExecutionSample event is mostly legacy. When JFR was created, there were only four settings supported in the JMX API and the file format: enabled, threshold, period, and stackTrace.

> And how should I document the new behaviour? Add it to the documentation of the throttle annotation?

The throttle annotation is not a public API. I would add it to the description of the event.

> How should I treat the `off` setting? I disabled the rate setting for now because it doesn't make any sense when the period can be arbitrarily small. Of course, I could replace it with the smallest reasonable value of "1ms". Any opinions?

Hmm, not sure. For now, keep it disabled.

> The only place where the current throttle syntax is defined seems to be the throttle annotation.

Long term, the correct place is jdk/jfr/package-info.java, but we haven't done it for "throttle" because we are not sure how it will behave with other features in the pipeline.

> Ok, so is the following ok?
> 
> ```
> <Event name="CPUTimeSample" category="Java Virtual Machine, Profiling" label="CPU Time Method Sample"
>     description="Snapshot of a threads state from the CPU time sampler. The throttle can be either an upper bound for the event emission rate, e.g. 100/s, or the cpu-time period, e.g. 10ms, with s, ms, us and ns supported as time units."
>     period="everyChunk" throttle="true" experimental="true">
>     <Field type="Thread" name="sampledThread" label="Thread" />
>     <Field type="StackTrace" name="stackTrace" label="Stack Trace" />
>     <Field type="boolean" name="failed" label="Failed" description="Failed to obtain the stack trace" />
>     <Field type="Tickspan" name="sampleTime" label="CPU time sampling period for this event"/>
>   </Event>
> ```

Looks reasonable, but the label for cpuTime needs to be fixed. We try to avoid using the word "event" in labels, and it should be short and use headline-style capitalization ([Label javadoc](https://docs.oracle.com/en/java/javase/23/docs/api/jdk.jfr/jdk/jfr/Label.html)).

Maybe we should call the field samplingPeriod? Hmm, need to think about this.

I also wonder if we should use the event thread as sampledThread. The plan is to move over to cooperative sampling, so the event will be emitted in the thread that was sampled. This is also true for ExecutionSample. We may need to keep the field name for compatibility reasons for that event, or have both.

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

PR Comment: https://git.openjdk.org/jdk/pull/20752#issuecomment-2379731752
PR Comment: https://git.openjdk.org/jdk/pull/20752#issuecomment-2416769842
PR Comment: https://git.openjdk.org/jdk/pull/20752#issuecomment-2416780937
PR Comment: https://git.openjdk.org/jdk/pull/20752#issuecomment-2416953141
PR Comment: https://git.openjdk.org/jdk/pull/20752#issuecomment-2419043811