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. src/java.base/share/classes/jdk/internal/ffi/util/FFMUtils.java line 51: > 49: public static final AddressLayout C_POINTER = ValueLayout.ADDRESS > 50: .withTargetLayout(MemoryLayout.sequenceLayout(Long.MAX_VALUE, > JAVA_BYTE)); > 51: public static final ValueLayout.OfLong C_LONG = (ValueLayout.OfLong) > Linker.nativeLinker().canonicalLayouts().get("long"); This seems problematic. On Windows, the C type `long` has a `JAVA_INT` layout, because it only uses 32 bits. The reason jextract re-generates all these constants is that they can, in general, vary by platform, so my general feeling is that we can't just have a shared set of layout constants at these level -- the constants will need to be added in the platform-specific directories. If you want to make this layer more portable, then the `C_LONG` constant should be dropped, and clients should use either `C_INT` or `C_LONG_LONG` (which is what most portable C APIs end up doing anyway). Another possible way to have a more robust shared layer is to use definitions in stdint.h -- e.g. not `C_INT`, but `C_INT32_T` (but then you might have issues, as the jextract-generated files you are importing depend on names like `C_INT`). src/java.base/share/classes/jdk/internal/ffi/util/FFMUtils.java line 99: > 97: .or(Linker.nativeLinker().defaultLookup()); > 98: > 99: public static void traceDowncall(String name, Object... args) { The tracing downcal support is also something that might be moved directly to the Linker -- either as a Linker option, or as a global flag, similar to `Xcheck:jni` (not in this PR) src/java.base/share/classes/jdk/internal/ffi/util/FFMUtils.java line 106: > 104: } > 105: > 106: public static MemorySegment findOrThrow(String symbol) { Hopefully this can go away once we update jextract to a new version that doesn't require this (not in this PR): https://github.com/openjdk/jextract/pull/280 src/java.base/share/classes/jdk/internal/ffi/util/FFMUtils.java line 118: > 116: } > 117: > 118: public static MemoryLayout align(MemoryLayout layout, long align) { As observed in the past, this `align` method could probably just be added on `MemoryLayout` (not as part of this PR, this is just a general observation). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2244889620 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2244899130 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2244904140 PR Review Comment: https://git.openjdk.org/jdk/pull/25546#discussion_r2244897270
