On Thu, 5 Jun 2025 00:21:45 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> We need some indication of which events are throttleable and looking at the >> mirror event may not work in some scenarios. >> >> We need to sample the endTime, because the startTime may be several minutes >> in the past. We could use commit(startTime, endTime, ...) and calculate the >> duration inside the method, but it may be confusing because the fields in >> the event are startTime and duration. We would also need to calculate the >> duration twice, both for shouldCommit and commit. >> >> When we get value types and perhaps value events, much of the ugliness of >> static methods could be avoided. > > I think we (from both the java.base and jdk.jfr perspectives) need to keep an > eye on the complexity of the use sites. The new throttling stuff requires a > new local variable. By itself this isn't a big deal, but there are 12 events > being updated here. In addition, each requires a start, end, and duration, > and clearly duration = end - start. These are all long values, and the > different calls require different long values, so sooner or later somebody is > going to get this wrong. > > To a certain extent we can do more cleanup on the java.base side, by using > things like SocketReadEvent's offer() method instead of its emit() method. > Unfortunately only one site uses offer() -- NIO SocketChannelImpl -- and note > that it didn't need to be updated! The other events should have something > like the offer() method, which groups together the testing of > shouldCommit/shouldThrottleCommit with the committing of the event. (This > also should avoid recalculating the duration, but really, this is only a > subtraction of two longs, so it should take only one cycle.) > > But note what we're doing here is constructing an internal API within > java.base, between the use sites (like java.net.Socket) and the > java.base-side JFR stuff (jdk.internal.event.SocketReadEvent). Maybe after > things are cleaned up and consolidated here we can see if the API between > jdk.internal.event and jdk.jfr can be improved. I updated FileRead and FileWrite to use an offer method like SocketRead and SocketWrite, reducing boilerplate. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2128398824
