Review request for JMC-6492: Add unit support for jdk.jfr.Frequency

[Erik Gahlin]

Den 05/06/19 kl. 18:47, skrev Jie Kang:
> On Wed, Jun 5, 2019 at 12:21 PM Erik Gahlin <erik.gahlin at oracle.com> wrote:
>> Frequency can also be used in combination with DataAmount.
>>
>> For example, the Network Utilization event can have a rate of 100 Mbps
>> using the annotations @Frequency @DataAmount(DataAmount.BITS) in
>> combination.
> For what it's worth, JMC-6348 [1] is an open issue for supporting
> multiple content types such as in the NetworkUtilization event. I took
> a brief look in the past and I expect work on it in the next few
> months but not likely from myself, unfortunately.
>
> [1] https://bugs.openjdk.java.net/browse/JMC-6438
>
>> I am thinking of adding two units, Frequency.HERTZ and
>> Frequency.PER_SECOND.
> I'm a little confused of the difference between the two; isn't Hertz
> unit s^-1, also "per second"?

"unit" was perhaps not the right word, but 1000 with

@Frequency("HERTZ") would result in "1 kHz" while

@Frequency("PER_SECOND") would become 1 k/s", "1000 kps" or perhaps 
"1000 /s"

The first form could be used with CPUTimeStampCounter and the second 
form would work well with  ExceptionStatistics or ThreadContextSwitchRate.

DIdn't know about the other open issue, so combinations is perhaps best 
done best done in that enhancement.

Erik
>
>
>> Question is, what should be the default?
>>
>> I'm leaning towards PER_SECOND as it seems more common. This is not for
>> JDK 13, as it requires a CSR request and some bake time.
>>
>> Erik
>>
>>> Hi Jie,
>>>
>>> On Wed, May 29, 2019 at 9:36 AM Jie Kang <jkang at redhat.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> The attached patch adds a case for contentType "hertz",  annotation
>>>> "jdk.jfr.Frequency" used in JFR events like CPUTimeStampCounter, to
>>>> use the Hertz unit introduced in JMC-5768.
>>>>
>>>> The following is a before and after image for the event browser for
>>>> CPUTimeStampCounter
>>>> https://imgur.com/a/cSVlmOM
>>>>
>>>> How does it look?
>>>>
>>> This looks good to me.
>>>
>>> Just a note from playing around with the patch, CPUTimeStampCounter was
>>> updated in changeset 2cf5bec8d8ba [0] to use hertz instead of frequency per
>>> second. As a result, older recordings will still show their numerical value
>>> as per their jfr file, but newer recordings will now correctly use the hz
>>> unit.
>>>
>>>
>>>> Regards,
>>>>
>>> Cheers,
>>>
>>> Alex
>>>
>>> [0] http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/2cf5bec8d8ba
>>