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

[Stuart Marks]

On Fri, 17 May 2024 09:26:03 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> My main concern here is the addition of clutter (checking two flags, and creating two levels of nested "impl" methods) at every call site. We may need to rearrange our internal API for JFR (jdk.internal.events) in order to clean up the call sites without loading additional classes unnecessarily.
>> 
>> I think the main performance comparison to make is the impact on startup time of loading the additional FileReadEvent class. That is, to compare the startup time of the baseline with code that loads the FileReadEvent class, with JFR disabled in both cases. I don't know how to do this. Anyway, if loading of additional classes imposes unacceptable overhead, then that justifies doing more work to avoid it. That's separate from whether adding the `jfrTracing` flag as done in this PR is an effective way to do it.
>
> I think `if (jfrTracing && FileReadEvent.enabled())` would be more readable as it would avoid calling going through the traceXXX methods when the flag is enabled but the specific event is not enabled.

Collapsing the extra layer of methods and combining the test into

    if (jfrTracing && FileReadEvent.enabled())

would indeed keep things a little neater.

I'm still questioning the need for `jfrTracing` at all. There's the matter of how this boolean gets set and unset, and whether this is done in a way that comports with the memory model. Setting this aside, is the only benefit that it avoids loading an extra class at JVM startup time (without JFR)? In this case that would be the `FileReadEvent` class, which is the stub class in `jdk.internal.event`. Wouldn't this class be in the CDS archive, making it not worth the effort of bringing up this new `jfrTracing` mechanism simply to avoid loading it unnecessarily?

I observe that in JDK 22 some (but not all) of the event classes in `jdk.internal.event` seem to be present in the CDS archive. These include various virtual thread events.

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

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