Allow to customize NIO (channel) select strategies. #4955

daschl · 2016-03-09T13:47:46Z

This PR is still a work in progress, subject to be reviewed by @normanmaurer (and others who are interested, of course). Supersedes #4952

daschl · 2016-03-09T15:43:09Z

@buchgr a change similar to this would maybe fit your needs better? no need for a separate event loop and pluggable backoff mechanisms.

Scottmitch · 2016-03-09T17:55:47Z

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

+     * @return {@link NoopBackoffStrategy} as the default.
+     */
+    public BackoffStrategy backoffStrategy() {
+        return new NoopBackoffStrategy();


Make NoopBackoffStrategy a singleton?

Also if this is baked into NioEventLoop anyways should we just add another constructor to NioEventLoopGroup and accept a BackoffStrategy argument?

@Scottmitch I considered that too, the only problem I had with this is that we need to call it N times (once per loop) so one strategy isn't enough - we'd need some kind of factory and then I thought well this might be simpler.

I had the singleton for the Noop there already, I can add it back for sure :)

we'd need some kind of factory

Yip makes sense if the BackoffStrategy has state. However this approach seems more portable if we eventually want to consider this patch for EpollEventLoop too.

daschl · 2016-03-10T08:20:06Z

@NiteshKant I see, that makes sense. I kinda like the reset approach, what do you think about the last change?

I also incorporated the singleton and factory as @Scottmitch suggested.
I'm still thinking if we get this in we should not include the parking strategy since its very likely that the hardcoded parts are not good for every use case and to make it more tunable we need more experience on what to actually tune - unless someone comes up with a sane strategy and defaults.

I'm keeping in the PR for now so we can talk about potential extensions :)

daschl · 2016-03-10T08:21:35Z

@normanmaurer new day new update ;)

NiteshKant · 2016-03-10T18:05:27Z

@daschl yes the reset() in the strategy looks good 👍

Scottmitch · 2016-03-11T02:45:15Z

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

+ * Default {@link BackoffStrategy} which does not perform backoff and will immediately trigger
+ * the selector blocking.
+ */
+public class NoopBackoffStrategy implements BackoffStrategy {


make this class final and add a private constructor.

Scottmitch · 2016-03-11T03:08:02Z

@normanmaurer @trustin - Any thoughts about this?

@daschl - Can you also add a JMH benchmark demonstrating the benefits and ensuring this change doesn't regress for the "in event loop" use case?

daschl · 2016-03-11T07:20:47Z

@Scottmitch will rework it, but not sure about JMH (since there are many different workloads in theory that could be affected. I'll see if I can come up with something sane)

daschl · 2016-03-11T13:19:16Z

updated

Scottmitch · 2016-03-11T15:23:18Z

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

-                } else {
+                boolean hasTasks = hasTasks();
+                int selectNowCount = selectNow();
+                if (backoff.delaySelect(selectNowCount, hasTasks)) {


I thought about it a bit more and I think we can change the interface to replicate the existing behavior and also allow for extension. This strategy allows us to avoid an extra system call on every loop iteration in the default case and will also work with EpollEventLoop. By being able to retain the existing behavior it is less risky and much less you have to prove in terms of performance impact.

// This can go somewhere in common and is equivalent to Supplier from java 8, // but with a native type and we can't use Supplier from java 8 bcz // we must support java 6. public interface IntSupplier { int get(); } // BackofStrategy changes to SelectStrategy public interface SelectStrategy { // return value: -2 means try again, -1 means select, anything else is the result of selectNowSupplier.selectNow() and will run all tasks int calculateStrategy(IntSupplier selectNowSupplier, boolean hasTasks); } public final class DefaultSelectStrategy implements SelectStrategy { // this class should be a singleton. public int calculateStrategy(IntSupplier supplier, boolean hasTasks) { return hasTasks ? supplier.get() : -1; } } // In NioEventLoop: private final IntSupplier SELECT_NOW_SUPPLIER = new IntSupplier() { public int get() { return selectNow(); } }; .. selectloop: for(;;) { try { switch(selectStrategy.calculateStrategy(SELECT_NOW_SUPPLIER, hasTasks()) { case -2: continue selectloop; case -1: select(wakenUp.getAndSet(false)); if (wakenUp.get()) { selector.wakeup(); } // fall through default: cancelledKeys = 0; needsToSelectAgain = false; ... break; }

daschl · 2016-03-14T13:03:43Z

@Scottmitch updated the PR with your suggestions, let me know if that is what you are looking for :)

The only thing I'm not sure about is how we should handle a IOException in the supplier, right now I log the error, return 0 so that the code moves on (since there is not much at this point we can do, right?).

If that one looks good I'll get rid of the parking impl and polish it up so that we can get it ready for final review/merging?

daschl · 2016-03-14T13:04:38Z

@normanmaurer good time for you to review too I think.

normanmaurer · 2016-03-14T13:07:49Z

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

+ */
+public final class DefaultSelectStrategy implements SelectStrategy {
+
+    public static final DefaultSelectStrategy INSTANCE = new DefaultSelectStrategy();


s/DefaultSelectStrategy/SelectStrategy/

mp911de · 2016-03-17T14:20:52Z

I gave the change a try. Similar results for lettuce. A continuous loop using full request/response cycles on one thread results in a 5.5k ops/s increase (before the change ~ 16k ops/sec, after ~ 21.5k ops/sec)
In my tests

    @Override
    public int calculateStrategy(final IntSupplier supplier, final boolean hasTasks)
        throws Exception {
        return supplier.get();
    }

performed best (some 500 to 1000 ops/sec faster than the @daschl strategy).

Scottmitch · 2016-03-18T11:36:39Z

@daschl - No problem. I'll pickup the EPOLL portion.

@mp911de - Thanks for sharing!

trustin · 2016-03-23T05:27:37Z

Nice job! LGTM

Scottmitch · 2016-03-24T04:44:53Z

@normanmaurer - Any objections?

normanmaurer · 2016-03-24T05:19:09Z

Will check today and if not pull it in

Am 24.03.2016 um 05:44 schrieb Scott Mitchell notifications@github.com:

@normanmaurer - Any objections?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

normanmaurer · 2016-03-24T11:23:59Z

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

+                    case SelectStrategy.CONTINUE:
+                        continue selectloop;
+                    case SelectStrategy.SELECT:
+                        select(wakenUp.getAndSet(false));


@daschl should we do the same as before here:

- if (hasTasks()) { - selectNow(); - } else { - select(oldWakenUp);

technically that's what we have, right? and its up to the strategy to call selectNow(). Or am I misunderstanding you?

@normanmaurer - As @daschl said the intention is to replicate the existing behavior in DefaultSelectStrategy ... it seems this way to me but do you see a bug in the current code?

daschl · 2016-03-25T07:32:26Z

@normanmaurer why did this not make it into 4.0.35? :( :(

normanmaurer · 2016-03-25T07:34:42Z

@daschl will release 4.0.36.Final next week and include it. Just want to run some more benchmarks first

daschl · 2016-03-25T07:38:53Z

@normanmaurer sounds good to me, thanks! (rebased)

Motivation: Under high throughput/low latency workloads, selector wakeups are degrading performance when the incoming operations are triggered from outside of the event loop. This is a common scenario for "client" applications where the originating input is coming from application threads rather from the socket attached inside the event loops. As a result, it can be desirable to defer the blocking select so that incoming tasks (write/flush) do not need to wakeup the selector. Modifications: This changeset adds the notion of a generic SelectStrategy which, based on its contract, allows the implementation to optionally defer the blocking select based on some custom criteria. The default implementation resembles the original behaviour, that is if tasks are in the queue `selectNow()` and move on, and if no tasks need to be processed go into the blocking select and wait for wakeup. The strategy can be customized per `NioEventLoopGroup` in the constructor. Result: High performance client applications are now given the chance to customize for how long the actual selector blocking should be deferred by employing a custom select strategy.

normanmaurer · 2016-03-25T19:19:41Z

@Scottmitch @daschl yeah LGTM. I just want to do some benchmarking before pulling this in next week.

Scottmitch · 2016-03-25T19:28:10Z

@normanmaurer - Ok SGTM. I'll pull together a PR for EPOLL too so we can keep them consistent in the next release.

normanmaurer · 2016-03-25T19:28:49Z

@Scottmitch SGTM... thanks!

Scottmitch · 2016-03-28T20:57:48Z

@normanmaurer - May be easier to benchmark with #5044 since it includes this PR and EPOLL updates.

normanmaurer · 2016-03-30T15:42:40Z

So I did some tests and this looks good... @nmittler anything you want to add here or you are satisfied as well ?

nmittler · 2016-03-30T16:42:36Z

common/src/main/java/io/netty/util/IntSupplier.java

+/**
+ * Represents a supplier of {@code int}-valued results.
+ */
+public interface IntSupplier {


Why not use javax.inject.Provider? Are you worried about performance of autoboxing?

mostly because I didn't know this existed and in java8 there is a java.util.function.Supplier which is not is not tied to injectors through convention or javadocs. I think I still prefer the IntSupplier if we can't use java8 classes...

@nmittler I agree with @Scottmitch here :)

nmittler · 2016-03-30T16:45:40Z

LGTM

Scottmitch · 2016-03-30T22:01:51Z

Cherry-picked 4.1 (5d76daf) 4.0 (24cb673)

Thanks @daschl !

daschl · 2016-03-31T04:10:36Z

🎉

daschl mentioned this pull request Mar 9, 2016

Allow NioEventLoop extension #4952

Closed

daschl force-pushed the backoff branch 2 times, most recently from 4e18e9e to e8fa1b8 Compare March 9, 2016 15:42

Scottmitch reviewed Mar 9, 2016
View reviewed changes

daschl force-pushed the backoff branch from e8fa1b8 to 97632a3 Compare March 10, 2016 08:17

Scottmitch reviewed Mar 11, 2016
View reviewed changes

Scottmitch self-assigned this Mar 11, 2016

daschl force-pushed the backoff branch from 97632a3 to 00b768c Compare March 11, 2016 10:12

Scottmitch reviewed Mar 11, 2016
View reviewed changes

daschl force-pushed the backoff branch 2 times, most recently from 9d00065 to 4bb27e9 Compare March 14, 2016 13:02

normanmaurer reviewed Mar 14, 2016
View reviewed changes

normanmaurer modified the milestones: 4.0.35.Final, 4.0.36.Final Mar 21, 2016

normanmaurer reviewed Mar 24, 2016
View reviewed changes

daschl force-pushed the backoff branch from cb35b66 to c9b3a22 Compare March 25, 2016 07:41

Scottmitch mentioned this pull request Mar 25, 2016

EPOLL SelectStrategy #5044

Closed

nmittler reviewed Mar 30, 2016
View reviewed changes

Scottmitch assigned Scottmitch and unassigned daschl Mar 30, 2016

Scottmitch closed this Mar 30, 2016

This was referenced Apr 15, 2016

Allow to adjust timeout of Selector.select(timeout) via System Property #568

Closed

Throughput is low even with batching #5152

Closed

Allow to customize NIO (channel) select strategies. #4955

Allow to customize NIO (channel) select strategies. #4955

Conversation

daschl commented Mar 9, 2016

daschl commented Mar 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschl commented Mar 10, 2016

daschl commented Mar 10, 2016

NiteshKant commented Mar 10, 2016

Choose a reason for hiding this comment

Scottmitch commented Mar 11, 2016

daschl commented Mar 11, 2016

daschl commented Mar 11, 2016

Choose a reason for hiding this comment

daschl commented Mar 14, 2016

daschl commented Mar 14, 2016

Choose a reason for hiding this comment

mp911de commented Mar 17, 2016

Scottmitch commented Mar 18, 2016

trustin commented Mar 23, 2016

Scottmitch commented Mar 24, 2016

normanmaurer commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschl commented Mar 25, 2016

normanmaurer commented Mar 25, 2016

daschl commented Mar 25, 2016

normanmaurer commented Mar 25, 2016

Scottmitch commented Mar 25, 2016

normanmaurer commented Mar 25, 2016

Scottmitch commented Mar 28, 2016

normanmaurer commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmittler commented Mar 30, 2016

Scottmitch commented Mar 30, 2016

daschl commented Mar 31, 2016