On Thu, 30 Apr 2026 07:42:53 GMT, Alan Bateman <[email protected]> wrote:
>> Robert Toyonaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review feedback
>
> 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).
Thank you for your feedback @AlanBateman!
> The tests for the Socket and channels code run in tier2 (not tier1).
I just finished running tier 2 too (on linux amd64) and don't see regressions.
> 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 { .. }`.
Yes, good idea. I've done that suggested refactoring
> 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.
Added a blank line after those static fields
> 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.
Okay I have adopted your suggestion. Thanks!
> 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.
Good point. I have adopted your suggestion
> 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.
Okay I have removed the `catch` and call accept in the main thread now.
> 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().
Done!
-------------
PR Comment: https://git.openjdk.org/jdk/pull/30948#issuecomment-4357166828
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3171581219
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3171582339
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3171583576
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3171586913
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3171585703
PR Review Comment: https://git.openjdk.org/jdk/pull/30948#discussion_r3171585922
