NIO2: Call accept immediately if connection count is not close to max con…

[anilgursel]

…nections

Under regular load, we observe NIO2 has better CPU utilization and slightly better latency compared to NIO1. However, under heavy load or during spikes, we observed that the 99th percentile latency of NIO2 is significantly worse compared to NIO1.

• We see that the latency is worse compared to NIO1 only for the requests that end up establishing a new connection.
• When maxKeepAliveRequests=-1, we do not observe the latency degradation with NIO2 at all. But, we need and use Tomcat’s maxKeepAliveRequests feature, so setting it to -1 is not an option.
• The problem occurs only during spikes/high load, e.g., maxThreads=40, while the concurrent users sending requests is 100.
• The issue happens only for blocking scenarios, where Tomcat threads are blocked during request processing. When non-blocking programming is used (where Tomcat threads are freed quickly), we do not see any degradation compared to NIO1. However, we have many applications that would not be able to adopt non-blocking anytime soon.

While looking at Tomcat code, we saw that the accept is scheduled to be run on the thread pool. Our hypothesis was that this scheduling was causing the accept to be delayed significantly under heavy load. So, for a request with new connection establishment, it needs to go through the TaskQueue twice. So, just for testing purpose we set maxConnections=-1 to see the impact of immediate accept call (and in our test the number of concurrent connections does not get close to maxConnections), we observed that the 99th percentile latency problem with NIO2 disappeared.

Accordingly, proposing a change to call accept immediately if the number of connections are not close to maxConnections.

[rmaucher]

maxConnections overall is a bad design for NIO2, that is a bit obvious. It started with connections not being counted accurately then after some iterative refactorings of close it remains a bad idea. For the longest time the default was fixed to -1 (still there in 8.5).

I guess all you're doing is add a heuristic that works for you [but actually no guarantee]. Also about accept, I refactored it NIO2 style to save one thread and it seems more efficient overall, before it was a much more classical NIO1 style accept using the same dedicated thread (so one extra thread, and likely less efficient). The downside of the strategy is that the accept is just like the other processing threads when maxConnections is used. If there's a heavy thread starvation, then accept will get delayed. I don't think it's the end of the world, the server will actually be slow for everyone, but I understand. OTOH, if your app is as you say using non blocking IO and is saving on threads, then there's no downside to the strategy [even with full use of the thread pool, the threads become available very frequently and accept stays responsive].

So I'm ok with adding some heuristic, but maxThreads can be too low. Maybe maxConnections/2 would be good enough, with another heuristic on a minimal number ... Not very clean ...

[anilgursel]

I updated the PR to have it as getMaxConnections() / 2. I agree it is just a heuristic and not clean. We can potentially make it configurable?

If my understanding is correct, one reason to schedule the accept is to prevent blocking the thread. The reason behind blocking (via LimitLatch) is to have a hard limit on the maxConnections. Is there any possibility to relax this a bit? Something like:

else if (getConnectionCount() < getMaxConnections()) {
    increment();
    serverSock.accept(null, this);
}

With this, there is possibility of number of connections to go over maxConnections little bit (which I think the expectation can be managed in documentation). Actually, I believe there will only be one thread doing accept logic at any given time. Because, a new accept is scheduled only when the previous one is completed. Is this true? If so, it would not go beyond maxConnections.

[rmaucher]

Hum, ok, since incrementing is not done in parallel, then it's something that can be tried.

[rmaucher]

Hum, ok, since incrementing is not done in parallel, then it's something that can be tried. This will be in 10.0.0-M10 and 9.0.40.

[anilgursel]

Thanks @rmaucher!