8360025: (se) Convert kqueue Selector Implementation to use FFM APIs #25546
Conversation
|
👋 Welcome back dclarke! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@DarraghClarke The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
| * questions. | ||
| */ | ||
|
|
||
| package jdk.internal.ffi.util; |
There was a problem hiding this comment.
If this is some general ffm utils for using ffm, you can put it in jdk.internal.foreign.
There was a problem hiding this comment.
These methods are only intended to be used for "panamization" and are not for general use. So, this is the reason we propose to have them here. But maybe some of them might be of general interest?
src/java.base/macosx/classes/jdk/internal/ffi/generated/kqueue/kqueue_h.java
Outdated
Show resolved
Hide resolved
src/java.base/macosx/classes/jdk/internal/ffi/generated/kqueue/kqueue_h.java
Outdated
Show resolved
Hide resolved
src/java.base/macosx/classes/jdk/internal/ffi/generated/kqueue/kqueue_h.java
Outdated
Show resolved
Hide resolved
|
|
||
| // filters | ||
| static final int EVFILT_READ = -1; | ||
| static final int EVFILT_WRITE = -2; | ||
| static final int EVFILT_READ = kqueue_h.EVFILT_READ(); |
There was a problem hiding this comment.
Could we remove these constants and use kqueue_h constants directly?
| public static final ValueLayout.OfDouble C_DOUBLE = ValueLayout.JAVA_DOUBLE; | ||
| public static final AddressLayout C_POINTER = ValueLayout.ADDRESS | ||
| .withTargetLayout(MemoryLayout.sequenceLayout(Long.MAX_VALUE, JAVA_BYTE)); | ||
| public static final ValueLayout.OfLong C_LONG = ValueLayout.JAVA_LONG; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think this is a good idea and would allow us to make BindingUtils shared,
Though I wonder if it's best to move BindingUtils to be shared, or if its worth merging it into FFMUtils. I'd favour the second option
|
Some of the files are missing a blank line at the end. |
| @@ -34,13 +38,13 @@ class KQueuePoller extends Poller { | |||
| private final int kqfd; | |||
| private final int filter; | |||
| private final int maxEvents; | |||
| private final long address; | |||
| private final MemorySegment pollArrayRegions; | |||
There was a problem hiding this comment.
This is a poll array so I think very confusing to see "Regions" in the name.
| result = kqueue_h.kevent( | ||
| kqfd, keventMS, 1, NULL, | ||
| 0, NULL); | ||
| } while ((result == KQUEUE_ERROR_VALUE)); |
There was a problem hiding this comment.
The only case where the syscall should be restarted is EINTR.
| if (result == -errno_h.EINTR()) { | ||
| return IOStatus.INTERRUPTED; | ||
| } else { | ||
| throw new IOException("kqueue_poll failed"); |
There was a problem hiding this comment.
The syscall is "kqueue" rather than "kqueue_poll".
DarraghClarke
changed the title
openjdk
bot
changed the title
|
/reviewer 2 |
|
@AlanBateman Only the author (@DarraghClarke) is allowed to issue the |
|
I plan to review this but may be a week or two before I can get to it. |
|
/reviewers 2 |
|
@AlanBateman |
This PR aims to Panamize the Java Kqueue implementation, This is based on the work that was previously shared in #22307 , The main change since then is that this branch takes advantage of the changes made in #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.
Progress
Error
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25546/head:pull/25546$ git checkout pull/25546Update a local copy of the PR:
$ git checkout pull/25546$ git pull https://git.openjdk.org/jdk.git pull/25546/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25546View PR using the GUI difftool:
$ git pr show -t 25546Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25546.diff
Using Webrev
Link to Webrev Comment