On Wed, 4 Jun 2025 16:04:08 GMT, Erik Gahlin <egah...@openjdk.org> wrote:
>> src/java.base/share/classes/java/net/Socket.java line 970:
>>
>>> 968: long end = SocketReadEvent.timestamp();
>>> 969: long duration = end - start;
>>> 970: if (SocketReadEvent.shouldThrottleCommit(duration, end)) {
>>
>> The use sites want to ask if an event should be committed. Does the word
>> "Throttle" need to be in name as I don't think the use cases need to care
>> about this.
>>
>> Also, just to point out that the shouldXXX method is called with the the
>> end timestamp and duration whereas the offset/emit/commit methods are called
>> with the start timestamp and duration. So two long timestamps and a long
>> measure of time, easy to get it wrong somewhere. Maybe not this PR but I
>> think would be clearer at the use sites to use start or end consistently and
>> reduce potential for mistakes.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2127716516
