activemq-artemis
activemq-artemis copied to clipboard
apache
ARTEMIS-3163 Experimental support for Netty IO_URING incubator
I got interested by #3479 and decided to test those changes out against a "fresh" build of the broker. I found it quite impressive and decided to implement what was discussed on @franz1981 original pr in hopes it would interest someone else. I take no credit for this PR, as it contains almost exclusively the original changes and what was discussed in #3479.
One thing I did actually change was to default the amount of "remotingThreads" to 1 instead of "3 * CPUCount" (for IO_URING) based on suggestions in the original PR as well as here: netty-incubator-transport-io_uring/issues/106#issuecomment-876173958
According to my own testing this is what the default value should be, with only a few "extremes" benefiting from a few more.
I have seen some impressive results from using IO_URING as the transport that I should be able to share after a few more rounds of testing.
Thanks, but again, it's primarily your work 🙂
It would be interesting to see how the journal would perform under IO_URING as well, but that seems to be a significantly larger change.
As discussed on the offshoot PR on Franz' fork repo back then (https://github.com/franz1981/activemq-artemis/pull/15), I'd still be inclined to make the deps optional/provided even now, especially given it is disabled by default, would be untested, and most wont use it whilst those who do can easily add it.
I'm also not sure it makes sense to include hard deps on the incubator artifacts at this late stage when Netty 4.2 will actually bring them into the main repo (https://github.com/netty/netty/tree/4.2/transport-native-io_uring) and so changes their GAVs significantly, which seems likely to cause users friction with a hard dep present.
@gemmellr Oh, I completely missed the discussion on the forked repo. I agree that if the Netty 4.2 release is close it makes sense to wait for it.
Does anyone have an idea of when it might reach a stable release? It looks like it will enter beta stage next?
If the Netty release and it's adoption into Artemis is some time away though, I think that marking these dependencies as optional would largely make this PR unnecessary. Probably very few (if any) will take the time to work that into their deploys and as such it wont generate any usage data/feedback either.
So I think we should either: Wait for the Netty release (closing/holding this PR) or go with the incubator dependencies included (until we move to Netty 4.2).
Personally I wouldnt do either of those. I would either proceed without using hard deps, or wait for 4.2 without hard deps. Given the Pr has sat for 3.5 years at this stage, I would probably go for the latter personally, unless someone knows 4.2 will be ages, but so far it doesnt seem like it.
In both cases I think its premature including the deps for all client and broker users when most wont use it as its disabled, and given that we wouldn't actually be testing it at all. A hard dep with either 4.1 or 4.2 cases will also likely add user friction as consuming builds straddle the 4.1/4.2 transition, which is just another reason not to add a hard dep for me when its easy for those who want it to add it.
I would suggest waiting until 4.2 is released and the feature becomes part of netty proper and has the testing behind it to ensure a reasonable user experience along with sufficient testing.
@jbertram Thanks for the heads up!
It seems like the transport classes for IO_URING have yet to be uploaded to maven but I went ahead and started working on this regardless, using the RC4 release in the meantime (for the transport classes, otherwise using the released 4.2.0)...
The upgrade to Netty 4.2.0 seem to introduce some issue, at least with the cli "Perf Client" I intended to use for some AB-testing... I'm still trying to figure that out.
Just bumping netty to 4.2.0, building, creating a default broker, then running:
bin/artemis perf client --warmup 20 --duration 60 --max-pending 10 --url tcp://localhost:61616?confirmationWindowSize=20000 queue://QUEUE
Shows the issue after some 5-10 seconds. Might be because the perf client is using the now deprecated "DefaultEventLoopGroup", not sure yet...
I saw that dependabot pushed for this Netty upgrade so I'll paste this comment in that PR just to be safe while looking into this.
FWIW, I've got a branch named netty42x where everything appears to be working with Netty 4.2.x. I tested with the perf client command you referenced and I didn't see any issues.