On Sun, 12 May 2024 07:27:26 GMT, Alan Bateman <al...@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. > >> 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1599077056
