RFR: 8331876: JFR: Move file read and write events to java.base

[Erik Gahlin]

On Thu, 9 May 2024 07:20:55 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Hi,
>> 
>> Could I have a review of a change that moves the jdk.FileRead and jdk.FileWrite events to java.base to remove the use of the ASM instrumentation.
>> 
>> Testing: jdk/jdk/jfr
>> 
>> Thanks
>> Erik
>
> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 78:
> 
>> 76: 
>> 77:     // Flag that determines if file reads/writes should be traced by JFR
>> 78:     private static boolean jfrTracing;
> 
> Should the force method be changed to test this flag too?
> 
> I'm also wondering about the transferXXX methods. We might want to think about these for a separate PR as they have more potential to be outliers than the read/write methods.

I think it would be good to use the flag for all events, but I rather do it as separate PR so this is mostly a mechanical change to remove ASM. It makes it easier to track regressions or improvements.

I can file an enhancement for the transferXXX methods.

> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 233:
> 
>> 231:     }
>> 232: 
>> 233:     private int readImpl(ByteBuffer dst) throws IOException {
> 
> Would you mind renaming these to implXXX rather than XXXImpl? The implXXX is the convention in this area (including in the API).
> 
> For placement, the read methods are grouped, and the write methods are grouped, only to avoid needing to jump around the file when touching this code. So I think easier if the methods that wraps implRead be located with the read methods.

Will fix.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595080275
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595080544