RFR: 8331876: JFR: Move file read and write events to java.base [v3]

[Stuart Marks]

On Thu, 9 May 2024 14:59:34 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

>> src/java.base/share/classes/java/io/FileInputStream.java line 63:
>> 
>>> 61:     private static final int DEFAULT_BUFFER_SIZE = 8192;
>>> 62: 
>>> 63:     // Flag that determines if file reads should be traced by JFR
>> 
>> It could be good to also note what will modify this flag - here and in other classes.
>> I'm going to guess that the purpose of this flag is purely performance, to avoid even loading the event class, `FileReadEvent` here, during startup/bootstrap and when JFR is not enabled, as read and write methods are highly performance sensitive? Otherwise the flag could live in the event class itself? Is it substantially faster to check this flag compared to `FileReadEvent.enabled()`?
>
> Its purpose is to avoid loading the FileReadEvent class. When the class is loaded, JFR will add fields and in some circumstances do other things. I don't think the cost is high, but it may add up if the number of events increases. Most Java applications don't run with JFR enabled, so this is to prevent them from seeing a negative impact.
> 
> I can update the text.

Hm, I think this setup requires more discussion. The approach we had settled on was that at the call sites in the libraries, something like the following was done:

    public R operation(...) {
        if (SomeEvent.enabled()) {
            // perform operation0 with tracing
            // emit event if SomeEvent.shouldCommit(...) is true
        } else {
            return operation0(...); // perform operation without tracing
        }
    }

    private R operation0(...) { /* do the actual work */ }

Now it looks like there's this additional flag `jfrTracing` that's set reflectively, and this flag is checked in a new layer of intermediate method calls:

    public R operation(...) {
        if (jfrTracing) {
            return traceOperation0(...);
        } else {
            return operation0(...);
        }
    }

    private R traceOperation0(...) {
        // stuff moved from public operation(...) above
    }

That is, the former body of the public `operation(...)` method is moved into the new `traceOperation0(...)` method.

I understand this is intended to help optimize startup time, but it adds clutter at each call site, and I'm wondering if it actually helps anything. The first time the application calls the `operation()` method, it's going to load a bunch of classes; the loading of this additional class is amortized over the loading and initialization of all the other classes in this area of the library. In addition, in the non-JFR case, the `enabled()` method implementation is simply `return false;` which can be inlined and which facilitiates dead code elimination.

With `jfrTracing` added to the mix, it causes a load from a non-final boolean field that needs to be checked repeatedly. Maybe the JIT can optimize for the common case, but there's possibly an expense that needs to be paid here.

At some point we should measure startup overhead for each case. I guess this can occur before or after this PR is integrated, depending on the urgency of things, but we should keep an eye on this issue.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1596129912