Introduce IoHandleEventLoopGroup / IoHandleEventLoop and its implemen… #13991

normanmaurer · 2024-04-21T17:23:31Z

…tations MultiThreadIoHandleEventLoopGroup / SingleThreadIoHandleEventLoop

Motivation:

At the moment each transport has its own EventLoopGroup and EventLoop implementation. Most of these are final because they are tightly coubled with the specific Channel implementations. This is unfortunate for multiple reasons, as for example:

It's hard to instrument EventLoop* implementations as these are not extensible at all or even if they are the same logic needs to be added to multiple EventLoop* implementations.
Registration / Deregistration is limited to Channels which remove flexibility as for example some Operation Systems allowing to also handle other IO (for example file IO) with something like an EventLoop (one example would be io_uring).
A lot of code-duplication between different EventLoop* implementations exists as most share kind of the same logic of running non IO tasks.

Modifications:

Add the concept of IoHandleEventLoopGroup / IoHandleEventLoop and the corrosponding IoHandle, IoHandler and IoHandlerFactory. This allows to only have a custom implement for IO handling per transport while share the same code for execution of tasks, scheduled task and ultimatly IO.
Add the implementations MultiThreadIoHandleEventLoopGroup and SingleThreadIoHandleEventLoop that can be used by all (or most) transport implementations by having these just provide their own IoHandler / IoHandlerFactory implementations.

Result:

More code sharing and easier of customizing EventLoops.

normanmaurer · 2024-04-21T17:24:56Z

I am opening this a draft mainly because I wanted to show and proof that we can back port some of the changes that we did in main without breaking too much things. I think even thought this PR introduce some API breaking it shouldn’t affect almost any user. To proof this I did not change how we create the different EventLoopGroups in our testsuite and everything still works.

So while the old way of constructing Eventloops still work the new way would be a bit different:

Old API:

new EpollEventLoop()

New API:

new MultiThreadIoHandleEventLoopGroup(EpollHandler.newFactory();

The benefit of the new way is that it is quite easy to instrument / doctorate an EventLoop/EventLoopGroup now. Basically you can just extend MultiThreadIoHandleEventLoopGroup / SingleThreadIoHandleEventLoop and add all the customization once while be able to reuse it with any transport implementation.
This includes things like:

able to get a count of registered channels / handles
understand how long the io processing vs task processing took.
how many channels / handles were processed per loop run
customize / decorate promises
…

Beside this IoHandleEventLoop can support not only Channel but also other subtypes of IoHandle. This would for example allow us to add file io support with io_uring and still use the same eventloop as for sockets etc. I am sure there are more things we could do by adding this kind of flexibility.

With all this said I still think the change is too dangerous for a new 4.1.x release. With this in mind I wondered if we might want to aim for releasing 4.2.0 with the following change:

This PR
Change the minimum java version to 8
Maybe a few more low risk changes.

I would love to hear what other project members and the community think about this ?

normanmaurer · 2024-04-21T17:26:13Z

@bryce-anderson @idelpivnitskiy @chrisvest @franz1981 @vietj @yawkat @trustin WDYT ?

normanmaurer · 2024-04-21T17:27:23Z

If interested I can also change all the creation code of EventLoopGroups in the testsuite and examples. I just wanted to not do this for now to show how small the impact for most people should be

…tations MultiThreadIoHandleEventLoopGroup / SingleThreadIoHandleEventLoop Motivation: At the moment each transport has its own EventLoopGroup and EventLoop implementation. Most of these are final because they are tightly coubled with the specific Channel implementations. This is unfortunate for multiple reasons, as for example: - It's hard to instrument EventLoop* implementations as these are not extensible at all or even if they are the same logic needs to be added to multiple EventLoop* implementations. - Registration / Deregistration is limited to Channels which remove flexibility as for example some Operation Systems allowing to also handle other IO (for example file IO) with something like an EventLoop (one example would be io_uring). - A lot of code-duplication between different EventLoop* implementations exists as most share kind of the same logic of running non IO tasks. Modifications: - Add the concept of IoHandleEventLoopGroup / IoHandleEventLoop and the corrosponding IoHandle, IoHandler and IoHandlerFactory. This allows to only have a custom implement for IO handling per transport while share the same code for execution of tasks, scheduled task and ultimatly IO. - Add the implementations MultiThreadIoHandleEventLoopGroup and SingleThreadIoHandleEventLoop that can be used by all (or most) transport implementations by having these just provide their own IoHandler / IoHandlerFactory implementations. Result: More code sharing and easier of customizing EventLoops.

He-Pin · 2024-04-22T07:09:45Z

Do you plan to align with the design in Netty 5?

transport-classes-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java

yawkat · 2024-04-22T08:06:44Z

transport-classes-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoopGroup.java

-        for (EventExecutor e: this) {
-            ((EpollEventLoop) e).setIoRatio(ioRatio);
-        }
+        // NOOP


Same here, need to document that this does nothing now

transport-classes-epoll/src/main/java/io/netty/channel/epoll/EpollHandler.java

transport/src/main/java/io/netty/channel/IoExecutionContext.java

transport/src/main/java/io/netty/channel/IoHandler.java

transport/src/main/java/io/netty/channel/nio/NioHandler.java

normanmaurer · 2024-04-22T14:11:19Z

Will look into the build timeout later today

normanmaurer · 2024-04-22T14:12:05Z

Do you plan to align with the design in Netty 5?

What design you talk about ? The ideas was to introduce the same concept with minimal real world breakage

He-Pin · 2024-04-22T14:24:22Z

Do you plan to align with the design in Netty 5?

What design you talk about ? The ideas was to introduce the same concept with minimal real world breakage

I mean split the ioHandler with the event loop, I will take some time to look into this.

normanmaurer · 2024-04-22T14:43:26Z

Do you plan to align with the design in Netty 5?

What design you talk about ? The ideas was to introduce the same concept with minimal real world breakage

I mean split the ioHandler with the event loop, I will take some time to look into this.

That's exactly what this PR does ;)

normanmaurer · 2024-04-22T14:46:19Z

Also I think if we would switch to java8 I could maybe do something more flexible to support different IoHandle in the future.

violetagg · 2024-04-22T14:50:04Z

@normanmaurer I don't know whether we should discuss this here in this PR but

With all this said I still think the change is too dangerous for a new 4.1.x release. With this in mind I wondered if we might
want to aim for releasing 4.2.0 with the following change:

This PR
Change the minimum java version to 8
Maybe a few more low risk changes.

Have you considered moving out of incubation some of the projects? (io_uring, QUIC, HTTP/3)?

transport/src/main/java/io/netty/channel/IoHandleEventLoop.java

normanmaurer · 2024-04-22T14:52:21Z

@normanmaurer I don't know whether we should discuss this here in this PR but

With all this said I still think the change is too dangerous for a new 4.1.x release. With this in mind I wondered if we might
want to aim for releasing 4.2.0 with the following change:

This PR
Change the minimum java version to 8
Maybe a few more low risk changes.

Have you considered moving out of incubation some of the projects? (io_uring, QUIC, HTTP/3)?

Yes but let's focus on this one first :)

He-Pin · 2024-04-22T15:23:33Z

I would like to +1 for moving to Java 8 on Netty 4.2.x, as Netty 4.1.x has a very long history, With Java 8's default method, lambda completableFuture, etc., we can achieve more than it is now.

We can submit a PR and wait to see if people complain about moving to Java 8 on Netty 4.2.x

pom.xml

normanmaurer · 2024-04-22T15:31:28Z

I would like to +1 for moving to Java 8 on Netty 4.2.x, as Netty 4.1.x has a very long history, With Java 8's default method, lambda completableFuture, etc., we can achieve more than it is now.

We can submit a PR and wait to see if people complain about moving to Java 8 on Netty 4.2.x

I think we should definitely try or even use 11. Let's discuss about this as a followup.

normanmaurer · 2024-04-24T02:54:16Z

I will port things over for for sure

transport-classes-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java

transport/src/main/java/io/netty/channel/nio/NioEventLoop.java

bryce-anderson · 2024-04-23T22:43:10Z

transport-classes-epoll/src/main/java/io/netty/channel/epoll/EpollHandler.java

+/**
+ * {@link EventLoop} which uses epoll under the covers. Only works on Linux!
+ */
+public class EpollHandler implements IoHandler {


Does this class need to be public?

Yes it needs to be... that said we should make it final. All the IoHandler implementations need to be public as you use these to create the factory to inject in the MultiThreadIoEventLoopGroup.

For example:

var group = new MultiThreadIoEventLoopGroup(NioHandler.newFactory());

Another possibility would be to not make these public but provide a IoHandlerFactory implementation that is public.

Like this:

var group = new MultiThreadIoEventLoopGroup(new NioHandlerFactory());

This factory would then instance the handles.

@chrisvest @He-Pin @yawkat not sure what is better ?

current approach seems fine to me

I see. The factory seems nicer as it limits the escape of additional API but it's not a huge deal either way.

As we are using Java 8 now, so final class with factory would be better.

transport/src/main/java/io/netty/channel/nio/NioHandler.java

transport/src/main/java/io/netty/channel/local/LocalIoHandler.java

…easier implementation of IoHandler subtypes without the need to also change the IoHandle

normanmaurer · 2024-04-27T00:12:19Z

@He-Pin @violetagg @bryce-anderson @idelpivnitskiy @chrisvest @franz1981 @vietj @yawkat @trustin I just pushed more changes... I think from an API point of view I am now consider this done (and everything compiles / pass tests).

So what changed from the original PR design:

Rename *IoHandleEventLoop* to *IoEventLoop* to make things less verbose
renamed method registerForIo(...) to just register(...) and let it return an IoRegistration. This registration can then be used to modify which events some is interested in.
Channel does not extend IoHandle anymore.
Added IoOpt interface and added implements for all transports. These IoOpt implementations can be used with the IoRegistration to manipulate for which events an IoHandle is interested to get notified.
IoHandle has now a handle(....) method that is called for the events and so we could decouple the specific IoHandler implementations completely from the respective Channel implementations.
Added deprecations

The nice thing about the new API is really that it is trivial for advanced users to write their own IoHandle implementations and register it with our existing implementations without the need to touch either the *IoEventLoop*s or the IoHandler implementations for the various transports. This allows a lot of more advanced use cases and would for example allow users to write their own File support for io_uring once we use the new API there without the need for us to do it ourselves and a lot of other things.

The concept that is used now with IoHandler and IoHandle is kind of similar to what you get when using libev. So it's a lower-level API that can be used and is also used by ourselves.

I know the changes are non-trivial so thanks to everyone who takes the time for review and providing feedback :)

…asses

chrisvest

A few comments

pom.xml

transport-classes-epoll/src/main/java/io/netty/channel/epoll/EpollChannelConfig.java

transport/src/main/java/io/netty/channel/IoHandler.java

transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java

transport/src/main/java/io/netty/channel/nio/NioIoHandler.java

normanmaurer · 2024-04-29T18:11:41Z

Thanks a lot for all the reviews.... This is now merged :)

…ucting it Motiviation: #13991 introduced a new way of how EventLoopGroups should be constructed. We should update our code to use the new way in examples and tests Modifications: Rewrite all tests / example to use new way of constructing Result: Cleanup

…ructing it (#14020) Motiviation: #13991 introduced a new way of how EventLoopGroups should be constructed. We should update our code to use the new way in examples and tests Modifications: Rewrite all tests / example to use new way of constructing Result: Cleanup

normanmaurer requested a review from chrisvest April 21, 2024 17:27

normanmaurer force-pushed the eventloop_rework branch from 2620422 to e9c9852 Compare April 21, 2024 17:37

normanmaurer requested review from trustin, violetagg, ejona86, bryce-anderson, idelpivnitskiy and franz1981 April 21, 2024 17:45

normanmaurer added 4 commits April 21, 2024 15:01

remove duplication

856352f

Fix epoll

3ae97bd

Cleanup

fe12c64

Fix pom

69be2de

yawkat approved these changes Apr 22, 2024

View reviewed changes

Fix test timeout

d315184

normanmaurer commented Apr 22, 2024

View reviewed changes

transport/src/main/java/io/netty/channel/IoHandleEventLoop.java Outdated Show resolved Hide resolved

He-Pin reviewed Apr 22, 2024

View reviewed changes

pom.xml Show resolved Hide resolved

bryce-anderson approved these changes Apr 24, 2024

View reviewed changes

normanmaurer added 4 commits April 26, 2024 15:06

Rework the coupling of IoHandler and Channel implementation to allow …

07c0150

…easier implementation of IoHandler subtypes without the need to also change the IoHandle

Remove old workaround

7b836ee

Add log of noop

2b225de

Mark things as deprecated

14f0e7d

normanmaurer added 7 commits April 26, 2024 17:22

Cleanup

0ba58cb

Fix LocalServerChannel

db9ce08

cleanup

e54429d

Consistent naming

216171b

Cleanup and clarify

d1db017

Fix visibility and make it possible to decorate IoRegistration sub-cl…

aa7c143

…asses

Merge branch '4.2' into eventloop_rework

4add0a4

chrisvest reviewed Apr 28, 2024

View reviewed changes

Address comments of chris

6aea38f

normanmaurer requested review from chrisvest and He-Pin April 29, 2024 11:01

chrisvest approved these changes Apr 29, 2024

View reviewed changes

normanmaurer merged commit 596a1f0 into 4.2 Apr 29, 2024
17 checks passed

normanmaurer deleted the eventloop_rework branch April 29, 2024 18:11

normanmaurer added this to the 4.2.0.Final milestone Apr 29, 2024

normanmaurer mentioned this pull request Apr 30, 2024

Replace use of old EventLoopGroup construction with new way of constructing it #14020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce IoHandleEventLoopGroup / IoHandleEventLoop and its implemen… #13991

Introduce IoHandleEventLoopGroup / IoHandleEventLoop and its implemen… #13991

normanmaurer commented Apr 21, 2024

normanmaurer commented Apr 21, 2024 •

edited

normanmaurer commented Apr 21, 2024

normanmaurer commented Apr 21, 2024

He-Pin commented Apr 22, 2024

yawkat Apr 22, 2024

normanmaurer commented Apr 22, 2024

normanmaurer commented Apr 22, 2024

He-Pin commented Apr 22, 2024

normanmaurer commented Apr 22, 2024

normanmaurer commented Apr 22, 2024

violetagg commented Apr 22, 2024 •

edited

normanmaurer commented Apr 22, 2024

He-Pin commented Apr 22, 2024 •

edited

normanmaurer commented Apr 22, 2024

normanmaurer commented Apr 24, 2024

bryce-anderson Apr 23, 2024

normanmaurer Apr 24, 2024

yawkat Apr 24, 2024

bryce-anderson Apr 24, 2024

He-Pin Apr 24, 2024

normanmaurer commented Apr 27, 2024 •

edited

chrisvest left a comment

normanmaurer commented Apr 29, 2024

Introduce IoHandleEventLoopGroup / IoHandleEventLoop and its implemen… #13991

Introduce IoHandleEventLoopGroup / IoHandleEventLoop and its implemen… #13991

Conversation

normanmaurer commented Apr 21, 2024

normanmaurer commented Apr 21, 2024 • edited

normanmaurer commented Apr 21, 2024

normanmaurer commented Apr 21, 2024

He-Pin commented Apr 22, 2024

yawkat Apr 22, 2024

Choose a reason for hiding this comment

normanmaurer commented Apr 22, 2024

normanmaurer commented Apr 22, 2024

He-Pin commented Apr 22, 2024

normanmaurer commented Apr 22, 2024

normanmaurer commented Apr 22, 2024

violetagg commented Apr 22, 2024 • edited

normanmaurer commented Apr 22, 2024

He-Pin commented Apr 22, 2024 • edited

normanmaurer commented Apr 22, 2024

normanmaurer commented Apr 24, 2024

bryce-anderson Apr 23, 2024

Choose a reason for hiding this comment

normanmaurer Apr 24, 2024

Choose a reason for hiding this comment

yawkat Apr 24, 2024

Choose a reason for hiding this comment

bryce-anderson Apr 24, 2024

Choose a reason for hiding this comment

He-Pin Apr 24, 2024

Choose a reason for hiding this comment

normanmaurer commented Apr 27, 2024 • edited

chrisvest left a comment

Choose a reason for hiding this comment

normanmaurer commented Apr 29, 2024

normanmaurer commented Apr 21, 2024 •

edited

violetagg commented Apr 22, 2024 •

edited

He-Pin commented Apr 22, 2024 •

edited

normanmaurer commented Apr 27, 2024 •

edited