RFR: 8284686: Interval of < 1 ms disables ExecutionSample events

[Erik Gahlin]

On Wed, 13 Apr 2022 12:45:15 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

>> Fixes the issue by rounding the period interval to the next full millisecond larger than zero.
>> 
>> The following from the bug report works now:
>> 
>> java -XX:StartFlightRecording=filename=flight.jfr,jdk.ExecutionSample#period=999us CLASS_FILE; jfr print --events jdk.ExecutionSample flight.jfr
>> 
>> It considers the period to be `1ms`, producing the recording as expected.
>
> You mentioned elsewhere that it fails in debug mode, I think it should work there as well. 
> 
> Did you try your change with tests in test/jdk/jdk/jfr? Might make sense to write a test, or add your use case to an existing test.
> 
> The documentation should say "1 ms" instead of "1ms". The latter is only used at command-line. 
> 
> You are changing the specification, so a CSR might be needed as well.

> @egahlin is my PR fine?

I discussed this with Markus and we're leaning towards not mentioning it in the documentation and instead keep it as an implementation detail. All settings are a best effort, not just the period, as it depends O/S, hardware etc. so the error could in practise be magnitudes more than a ms.

In the test, could you check the behaviour instead of using internal classes.

Perhaps something like this. I have not tested the code.

    package jdk.jfr.api.recording.event;
    
    import java.time.Duration;
    import jdk.jfr.Event;
    import jdk.jfr.FlightRecorder;
    import jdk.jfr.consumer.RecordingStream;
    
    /**
     * @test Tests that periodic events are not disabled when using a very short
     *       period
     * @key jfr
     * @requires vm.hasJFR
     * @library /test/lib /test/jdk
     * @run main/othervm jdk.jfr.api.recording.event.TestShortPeriod
     */
    public class TestShortPeriod {
    
        static class PeriodicEvent extends Event {
        }
    
        public static void main(String... args) {
            testNativeEventPeriod();
            testJavaEventPeriod();
            testExecutionSamplePeriod();
        }
    
        private static void testNativeEventPeriod() {
            try (var r = new RecordingStream()) {
                r.enable("jdk.JVMInformation").withPeriod(Duration.ofNanos(1));
                r.onEvent(e -> r.close());
                r.start();
            }
        }
    
        private static void testJavaEventPeriod() {
            Runnable hook = () -> {
                PeriodicEvent e = new PeriodicEvent();
                e.commit();
            };
            FlightRecorder.addPeriodicEvent(PeriodicEvent.class, hook);
            try (var r = new RecordingStream()) {
                r.enable(PeriodicEvent.class).withPeriod(Duration.ofNanos(1));
                r.onEvent(e -> r.close());
                r.start();
            }
            FlightRecorder.removePeriodicEvent(hook);
        }
    
        // The execution sample event doesn't use the standard mechanism
        // for periodic events
        private static void testExecutionSamplePeriod() {
            try (var r = new RecordingStream()) {
                r.enable("jdk.MethodExecutionSample").withPeriod(Duration.ofNanos(1));
                r.onEvent(e -> r.close());
                r.start();
            }
        }
    }

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

PR: https://git.openjdk.java.net/jdk/pull/8183