On Tue, 21 May 2024 22:41:12 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> 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. I don't think issue is so much the overhead of loading (one or more) additional classes without JFR, even if it causes a extremely small regression, but the added overhead added to JFR when trying to fix the regression. I did an experiment with: `if (FlightRecorder.enabled && FileReadEvent.enabled())` and it passes JFR tests and tier1/tier2. I don't think `FlightRecorder.enabled` needs to be used on every event class, but it would be good to use on event classes loaded during startup, both for safety reasons and to lower overhead of JFR startup. The `jfrTracing` flag works as well, but it is perhaps harder to comprehend and requires an additional static boolean in every class, which does clutter things up. I can push Alan's suggestion of uniting the checks into one if-statement. It may help to see how it looks. Virtual thread events are typically loaded in main, after JFR has started, and not an issue unless `jcmd JFR.start `is used, which is not that common. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1613687099
