On Thu, 9 May 2024 07:20:55 GMT, Alan Bateman <al...@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
