On Fri, 30 May 2025 12:00:28 GMT, Darragh Clarke <dcla...@openjdk.org> wrote:
> This PR aims to Panamize the Java Kqueue implementation, This is based on the > work that was previously shared in https://github.com/openjdk/jdk/pull/22307 > , The main change since then is that this branch takes advantage of the > changes made in https://github.com/openjdk/jdk/pull/25043 to allow for better > performance during errno handling. > > These changes feature a lot of Jextract generated files, though alterations > have been made in relation to Errno handling and performance improvements. > > I will update this description soon to include performance metrics on a few > microbenchmarks, though currently it's roughly 2% to 3% slower with the > changes, which is somewhat expected, though there are still a few ideas of > possible performance improvements that could be tried. Any suggestions or > comments in that area are more than welcome however. Some of the files are missing a blank line at the end. src/java.base/macosx/classes/jdk/internal/ffi/generated/BindingUtils.java line 45: > 43: public static final AddressLayout C_POINTER = ValueLayout.ADDRESS > 44: .withTargetLayout(MemoryLayout.sequenceLayout(Long.MAX_VALUE, > JAVA_BYTE)); > 45: public static final ValueLayout.OfLong C_LONG = ValueLayout.JAVA_LONG; This is `int` on Windows. So, we could perhaps use `Linker.nativeLinker().canonicalLayouts().get("long")` here? "size_t" and "wchar_t" are other canonical layouts that may differ across platforms. src/java.base/macosx/classes/jdk/internal/ffi/generated/kqueue/kqueue_h.java line 260: > 258: * } > 259: */ > 260: public static int kevent64(int kq, MemorySegment changelist, int > nchanges, MemorySegment eventlist, int nevents, int flags, MemorySegment > timeout) { Is this method ever used? src/java.base/macosx/classes/jdk/internal/ffi/generated/kqueue/kqueue_h.java line 261: > 259: */ > 260: public static int kevent64(int kq, MemorySegment changelist, int > nchanges, MemorySegment eventlist, int nevents, int flags, MemorySegment > timeout) { > 261: var adapted$ = kevent64.ADAPTED; Why do we do this intermediary step? What is the reason for bringing `kevent64.ADAPTED` on the stack, as it is only used once? I know this is a pattern of jextract's. src/java.base/macosx/classes/jdk/internal/ffi/generated/kqueue/kqueue_h.java line 263: > 261: var adapted$ = kevent64.ADAPTED; > 262: try { > 263: if (FFMUtils.TRACE_DOWNCALLS) { I wonder what the price of this `if` branch might be (if any)? The javac compiler probably sees the `public static final boolean TRACE_DOWNCALLS = false;` as constant foldable and can eliminate the code at compile time. src/java.base/macosx/classes/sun/nio/ch/KQueue.java line 44: > 42: */ > 43: > 44: class KQueue { This class could be `final` src/java.base/macosx/classes/sun/nio/ch/KQueue.java line 63: > 61: > 62: // filters > 63: static final int EVFILT_READ = kqueue_h.EVFILT_READ(); Could we remove these constants and use `kqueue_h` constants directly? src/java.base/macosx/classes/sun/nio/ch/KQueue.java line 121: > 119: kqfd, keventMS, 1, NULL, > 120: 0, NULL); > 121: } while ((result == -1)); Is there a constant for this magic `-1`? src/java.base/macosx/classes/sun/nio/ch/KQueue.java line 147: > 145: nevents, tsp); > 146: if (result < 0) { > 147: if (result == errno_h.EINTR()) { Shouldn't this be `result == -errno_h.EINTR()` instead? ------------- PR Comment: https://git.openjdk.org/jdk/pull/25546#issuecomment-2987207196 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2156432012 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2121817152 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2121808733 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2121802519 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2121817810 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2156409376 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2121812502 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2121782927
