Skip to content
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 #5296

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AntonRoskvist
Copy link
Contributor

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.

@franz1981
Copy link
Contributor

Well done and thanks @AntonRoskvist !!

@AntonRoskvist
Copy link
Contributor Author

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.

@michaelandrepearce
Copy link
Contributor

+100

@gemmellr
Copy link
Member

As discussed on the offshoot PR on Franz' fork repo back then  (franz1981#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.

@AntonRoskvist
Copy link
Contributor Author

@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).

@gemmellr
Copy link
Member

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.

@tabish121
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants