On Tue, 7 May 2024 19:32:57 GMT, Erik Gahlin <egah...@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.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595068674
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595058030
