On Mon, 27 Apr 2026 16:01:10 GMT, Robert Toyonaga <[email protected]> wrote:
> This PR guards usage of FileForce, SocketRead, and SocketWrite events with > `jfrTracing` to prevent those classes from being loaded when JFR is not in > use. This is the same technique as what's currently used with exception > events and FileRead/FileWrite events. > > I used NMT and a simple test app that exercises file force and socket IO > paths to check for a difference in memory usage. > **NMT Before:** > Classes=1188, Metadata used=966064 B, Class space used=93008 B > **NMT After:** > Classes=1182, Metadata used=943728 B, Class space used=89456 B > Note that the difference in amount used doesn't actually change amount > committed because the backing memory is pre-allocated in chunks with larger > granularity. > > Testing: > - new test test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java to > check the guards work properly > - tier 1 > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). These low level classes are typically invoked in the context of some library so the number of classes loaded is probably much higher than we might see with a minimal test case. The tests for the Socket and channels code run in tier2 (not tier1). src/java.base/share/classes/jdk/internal/event/JFRTracing.java line 50: > 48: enable(Class.forName("sun.nio.ch.SocketChannelImpl")); > 49: enable(Class.forName("sun.nio.ch.SocketInputStream")); > 50: enable(Class.forName("sun.nio.ch.SocketOutputStream")); I assume this is because these classes aren't accessible from jdk.internal.event. src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 637: > 635: > 636: @Override > 637: public void force(boolean metaData) throws IOException { AsynchronousFileChannel::force is so rarely used so may not be worth included in this PR. src/java.base/share/classes/sun/nio/ch/SocketInputStream.java line 80: > 78: public int read(byte[] b, int off, int len) throws IOException { > 79: int timeout = timeoutSupplier.getAsInt(); > 80: if (!jfrTracing || !SocketReadEvent.enabled()) { At some point we should refactor these methods to replace `if (!a || !b)` with easier to read `if (a && b) { .. } else { .. }`. src/java.base/share/classes/sun/nio/ch/SocketOutputStream.java line 39: > 37: // socket writes should be traced by JFR. > 38: private static boolean jfrTracing; > 39: private final SocketChannelImpl sc; In all the changes classes then we should have a clear break between static and instance fields, here a blank line after the static field would make it easier to eyeball. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 86: > 84: > 85: private static void exerciseFileForce() throws IOException, > ExecutionException, InterruptedException { > 86: File tmp = > Utils.createTempFile("TestEventsNotLoadedWithoutJfr", ".tmp").toFile(); No need to use `File` here as tils.createTempFile returns a Path. So no need for toFile and toPath in this method. Replacing it with `Files.createTempFile(Path.of(""), "TestEventsNotLoadedWithoutJfr", ".tmp")` will work just as well, and avoid the dependency. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 98: > 96: } > 97: try (FileChannel fc = FileChannel.open(tmp.toPath(), READ, > WRITE)) { > 98: data.clear(); The mix of data.flip() and data.clear() is a bit confusing here. It would be clearer to invoke flip before the first try block, then assert that the Future::get returns the expected number of bytes written, then invoke flip again before the second try block. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 134: > 132: serverThread.join(); > 133: } catch (Exception e) { > 134: throw new RuntimeException(e); There should be no need to wrap the exception, much simpler to just let this method throw. You can invoke accept synchronously after the connect. This will remove the need for serverThread and make it much simpler. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 140: > 138: private static void exerciseSocketChannelImpl() { > 139: try (ServerSocketChannel ssc = ServerSocketChannel.open()) { > 140: ssc.bind(new InetSocketAddress(HOSTNAME, 0)); It would be clearer to use InetAddress.getLoopbackAddress(). ------------- PR Review: https://git.openjdk.org/jdk/pull/30948#pullrequestreview-4203154006 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3166251376 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3166257375 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3166233038 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3166246801 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3166297202 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3166347850 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3166321315 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3166328548
