On Fri, 1 May 2026 00:57:03 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). > > Robert Toyonaga has updated the pull request incrementally with one > additional commit since the last revision: > > review feedback I assume you'll update the copyright headers. As regards the change then I think it's okay although I don't have a sense as to whether there will be any noticeable benefit. The force method is not widely used, and AsynchronousFileChannel is very rarely seen. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 90: > 88: String s = "short"; > 89: ByteBuffer data = ByteBuffer.allocate(s.length()); > 90: data.put(s.getBytes()); `ByteBuffer buffer = StandardCharsets.UTF_8.encode("short");` might be clearer. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 108: > 106: } > 107: } finally { > 108: Files.deleteIfExists(tmp); It's often better to not delete the file used by a test as it may be needed when diagnosing a test failure. For this test I think better to drop the try-finally-delete. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 130: > 128: } > 129: }); > 130: clientThread.start(); A simpler (and more reliable) test does not need clientThread. The connect method will establish the connection, it does not require a thread to be blocked in the accept method. This will allow to invoke server.accept after the client.connect and it will work fine. You'll find dozens of tests in the net and nio/channels directories that do this. On the reliability front it means an exception on the "client end" will cause the test method to throw. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 172: > 170: serverThread.join(); > 171: } catch (IOException |InterruptedException e) { > 172: throw new RuntimeException(e); No need to catch and throw RuntimeException. test/jdk/jdk/jfr/event/io/TestEventsNotLoadedWithoutJfr.java line 206: > 204: serverThread.join(); > 205: } catch (Exception e) { > 206: throw new RuntimeException(e); Same here, no need for the catch and throw of RuntimeException. ------------- PR Review: https://git.openjdk.org/jdk/pull/30948#pullrequestreview-4236515551 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3195925037 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3195839734 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3195852168 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3195854576 PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3195856468
