On Mon, 13 May 2024 21:00:10 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>>> If an event class is loaded before JFR is started, the event class needs to >>> be retransformed, but if it is loaded later, we can add instrumentation on >>> class load and avoid the retransformation. More happens when an event class >>> is loaded compared to ordinary class load, for example, a startTime field >>> is added. >>> >>> I did a JMH run and the difference between Event::enabled() and a boolean >>> flag was a fraction of nanosecond. >> >> There are instances of FIS/FOS created in initPhase1 for the standard >> streams so loading as few classes and executing as minimal as possible is >> good. RAF will typically be used early too as the zip code opens zip files >> with a RAF. So doing as little as possible is good. > > 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1604627812
