On Fri, 24 May 2024 15:45:07 GMT, Erik Gahlin <egah...@openjdk.org> wrote:
>> 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 the issue is so much the overhead of loading (one or more) > additional classes without JFR, even if it causes an extremely small > regression, but the added overhead to JFR when trying to fix the regression. > The cost of a JVMTI retransformation is perhaps 100 - 1000 times that of > loading a class in the CDS archive. > > 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 (ClassCircularityError) 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. Updated with Alan's suggestion to make the code more readable. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1617228065
