-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARTEMIS-3163 Experimental support for Netty IO_URING incubator #3479
base: main
Are you sure you want to change the base?
Conversation
33f6084
to
aacb59e
Compare
aacb59e
to
b4307e8
Compare
@@ -401,6 +406,21 @@ public ActiveMQThreadFactory run() { | |||
acceptorType = KQUEUE_ACCEPTOR_TYPE; | |||
|
|||
logger.debug("Acceptor using native kqueue"); | |||
} else if (useIoUring && CheckDependencies.isIoUringAvailable()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect io uring to be primary option later with fallback to epoll and then nio, as such maybe this should be first in order of preference. Its off by default but this way going forwards correct preference order would exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes :) correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn karaf and its feature checks :O it works for reflection too? :O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably add some metadata to artemis-server-osgi let it know the io.netty.incubator.channel.uring package usages are optional.
These are my results by using a single single-threaded acceptor for both clients and replication (on the live broker) to fairly compare epoll vs io_uring under load. NOTE: These are just preliminary results, so I won't share HW configuration or anything to make this reproducible, but it should give the magnitude of improvement offered by io_uring.
The profile data collected with https://github.com/jvm-profiling-tools/async-profiler/ are attached on https://issues.apache.org/jira/browse/ARTEMIS-3163 But the important bits are:
The io_uring version is far more efficient while using resources then epoll despite our replication process already try to batch writes as much as possible to amortize syscall cost: would be interesting to compare io_uring with some Open-OnLoad kernel bypass driver using epoll :P IMPORTANT: |
|
||
public static boolean isIoUringAvailable() { | ||
try { | ||
return Env.isLinuxOs() && (boolean) (Class.forName("io.netty.incubator.channel.uring.IOUring") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just call IOUring.isAvailable() like we do with kqueue and epoll, dont do this via reflection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not available via maven afaik so it's up to the users to compile it from source and use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<dependency> <groupId>io.netty.incubator</groupId> <artifactId>netty-incubator-transport-native-io_uring</artifactId> <version>0.0.3.Final</version> <classifier>linux-x86_64</classifier> </dependency>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its available. These is actually in the docs on the netty github
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow ok, will remove the reflection then, thanks to have checked!
@@ -420,6 +425,11 @@ | |||
format = Message.Format.MESSAGE_FORMAT) | |||
void unableToCheckEpollAvailabilitynoClass(); | |||
|
|||
@LogMessage(level = Logger.Level.WARN) | |||
@Message(id = 212079, value = "IoUring is not available, please add to the classpath or configure useIoUring=false to remove this warning", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put in correct place in order or the code. So people know what the high watermark for next code is without needing to hunt. All others are in asc order, please follow
@@ -395,6 +395,11 @@ | |||
format = Message.Format.MESSAGE_FORMAT) | |||
void unableToCheckEpollAvailability(@Cause Throwable e); | |||
|
|||
@LogMessage(level = Logger.Level.WARN) | |||
@Message(id = 212080, value = "Unable to check IoUring availability ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put in correct order place in file based on id. See other comment for why
@@ -112,6 +113,7 @@ | |||
public static final String NIO_ACCEPTOR_TYPE = "NIO"; | |||
public static final String EPOLL_ACCEPTOR_TYPE = "EPOLL"; | |||
public static final String KQUEUE_ACCEPTOR_TYPE = "KQUEUE"; | |||
public static final String IOURING_ACCEPTOR_TYPE = "IO_URING"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just IOURING we dont add under score seperator for KQueue or EPoll. Just also avoids any escaping issues on urls or other things in config urls or admin console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps Franz wanted to avoid a ring of IOUs...but more probably its just that io_uring actually has the underscore in its name whilst I dont think the others do. Reasonable point about URL based config...though I thought it was just a boolean there?
Docs need updating. |
@franz1981 whats status on this one? Im keen to merge it, happy to help contribute any last bits like slight code re-org on if statements, and docs that are needed myself, if needed next week. |
That seems a nice: i just would like to perform some better testing to help users to know which kernel version (and incubator version) to use. Let it parks here for a week and then we can move on and maybe decide with a public vote if people are interested and would like to know what it is/its purpose. We can have a call too with the community to explain it :) |
@franz1981, just a friendly reminder that it's been a week. This looks like a nice feature so it would be nice to get it out to the community if it's ready. |
I don't know yet if we should bring this in without any reflection usage (as @michaelandrepearce suggested) and we need to add a doc paragraph that state we have no idea (yet) if SSL or other features works as expected ie EXPERIMENTAL tag on everything. |
@@ -112,6 +113,7 @@ | |||
public static final String NIO_ACCEPTOR_TYPE = "NIO"; | |||
public static final String EPOLL_ACCEPTOR_TYPE = "EPOLL"; | |||
public static final String KQUEUE_ACCEPTOR_TYPE = "KQUEUE"; | |||
public static final String IOURING_ACCEPTOR_TYPE = "IO_URING"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps Franz wanted to avoid a ring of IOUs...but more probably its just that io_uring actually has the underscore in its name whilst I dont think the others do. Reasonable point about URL based config...though I thought it was just a boolean there?
@@ -61,6 +61,8 @@ | |||
|
|||
public static final String USE_KQUEUE_PROP_NAME = "useKQueue"; | |||
|
|||
public static final String USE_IOURING_PROP_NAME = "useIoUring"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your recent comments Franz noted flagging everything with caution that its experimental...this might be a simple spot to trivially bang the point home to a user, making the option name reflect it...e.g "useIoUringExperimental"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, i think what franz has here is fine/better, and that should just be clear in the docs, else we end up with mangled config , it will be off by default anyhow so someone will have to have read the docs to know its there of which it will be mentioned very clearly this is an incubator feature, and explicitly turned it on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having experimental bits using explicit non-final options or even APIs prior to completion to signal their status isnt mangled config IMO. Its fairly common, e.g the JDK has many instances of this. The explicit incubator GAV for the netty bit is another example.
@@ -401,6 +406,21 @@ public ActiveMQThreadFactory run() { | |||
acceptorType = KQUEUE_ACCEPTOR_TYPE; | |||
|
|||
logger.debug("Acceptor using native kqueue"); | |||
} else if (useIoUring && CheckDependencies.isIoUringAvailable()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably add some metadata to artemis-server-osgi let it know the io.netty.incubator.channel.uring package usages are optional.
I think we should run the extended suite using io_uring... if it's pass.. we should merge it upstream. |
@clebertsuconic i agree we should merge upstream asap, but we should be using the maven dependency and some docs are def needed before merge, @franz1981 my intent was to send you PR with changes and docs, I had a disaster work week, im hoping to get to it next week for you, actually just booked some hours out the calendar to try make sure i get the time. @franz1981 - Just a note on machines with Solarflare cards with kernal by pass with ONLOAD, perf improvement not really noticeable, but i borrowed a machine without special network kit, and there was a quite noticeable latency improvement and drop in resource usage for the same loads, essentially matching what your own tests had already shown, but it does mean its reproducable outside your lab the improvement |
@franz1981 i sent to your branch which this is PR from, a PR with my proposed changes to address the comments i had raised in this PR, see: franz1981#15 , if you agree with those then feel free to merge to your branch, and i have no further comments. Basically 3 key bits i addressed.
|
@franz1981, @clebertsuconic, @michaelandrepearce, where are we on this? Is this something we still want to do? Netty's io_uring lib is still in their incubator, but it's up to 0.16 now. |
@jbertram i would like to still see this in, as long as we mark it experimental or in incubation in docs, i personally support this. @franz1981, its on your branch atm this PR and i sent you those changes still pending, if you don't have time anymore, i can send in PR from my branch (closing this one), and merge/rebase on current master. |
@michaelandrepearce, given the silence from @franz1981 I'd say you should go ahead and send a PR of your own with the changes you recommended (plus resolving the existing conflicts). Then we can move forward. Thanks! |
https://issues.apache.org/jira/browse/ARTEMIS-3163