Dispatcher no longer has quadratic behaviour

[iamdanfox]

We had a production incident earlier today in which outgoing requests were blocked, with jstacks suggesting a lock in the Dispatcher as the culprit (on 3.12.0). (cc @swankjesse who worked on this recently)

This PR eliminates some quadratic behaviour in the Dispatcher by keeping track of requests per host.

I doubt this PR will really solve the problem we encountered (especially as we only use maxRequests = 256 and maxRequestsPerHost = 64), but it did seem a bit inefficient that the promoteAndExecute method iterated through the entire runningAsyncCalls for every item of readyAsyncCalls. I thought I'd put this PR up anyway as you might be interested in a perf improvement. Happy to add more test coverage if this is a direction you'd be interested in.

---

A very unscientific benchmark on my laptop showed speedups as maxRequests increased:

maxRequests | before   | after
64          | 30ms     | 30ms
640         | 101ms    | 71ms
6400        | 1266ms   | 196ms
64000       | 2minutes | 917ms

  @Test public void benchmark() throws Exception {
    int maxRequests = 640;
    dispatcher.setMaxRequestsPerHost(maxRequests);
    dispatcher.setMaxRequests(maxRequests);
    long before = System.currentTimeMillis();
    for (int i = 0; i < maxRequests; i ++) {
      client.newCall(newRequest("http://a/" + i)).enqueue(callback);
    }
    long after = System.currentTimeMillis();
    System.out.println("Elapsed = " + (after - before));
    assertEquals(executor.calls.size(), maxRequests);
  }

---

For reference, jstacks showed:

• 64 threads BLOCKED on 0x00007f28956a5000 line okhttp3.Dispatcher.finished(Dispatcher.java:219)
• 52 threads BLOCKED on 0x00007f28956a5000 on line okhttp3.Dispatcher.enqueue(Dispatcher.java:134)
• jstacks over ~5 seconds showed a RUNNABLE thread stuck holding the lock in the runningCallsForHost method:

"OkHttp https://redacted:redacted/..." #699949 prio=5 os_prio=0 tid=0x00007f2929116000 nid=0x9730 runnable [0x00007f28956a5000]
   java.lang.Thread.State: RUNNABLE
	at okhttp3.Dispatcher.runningCallsForHost(Dispatcher.java:198)
	at okhttp3.Dispatcher.promoteAndExecute(Dispatcher.java:175)
	- locked <0x00000000ab2ff958> (a okhttp3.Dispatcher)
	at okhttp3.Dispatcher.finished(Dispatcher.java:224)
	at okhttp3.Dispatcher.finished(Dispatcher.java:209)
	at okhttp3.RealCall$AsyncCall.execute(RealCall.java:218)
	at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)

[swankjesse]

Wow, that's both awful and unexpected. I assume the problem is that we're doing work while holding that lock that we shouldn't be. The dispatcher lock is intended to defend against memory accesses and should never be a contention point.

[swankjesse]

May I propose an alternate fix? Two broad ideas:

Take an optional host hint for the method and pass it when a call completes or is enqueued. If we have more than 1 general capacity then search for a call for that host specifically.

Add an AtomicInteger to RealCall which represents the running calls for that host. Initialize it when the call is enqueued to the AtomicInteger of another call that shares the host, or 0 if there are none.

[iamdanfox]

@swankjesse OK I can take a stab at that AtomicInteger version - is the motivation there to reduce the Map allocation or minimise the new abstractions in one PR? It seems a bit sad to add mutability to the otherwise immutable RealCall.AsyncCall...

[swankjesse]

Yeah I like this much better.

[swankjesse]

Probably needs to loop over ready calls also!

[iamdanfox]

Also, it seems that synchronous calls are not affected by the maxRequestsPerHost limit... I think I've preserved this behaviour, but is this desirable/correct?

[swankjesse]

Synchronous calls don't participate. You got it.

[swankjesse]

Nice!

[swankjesse]

Nice improvement. Presumably your benchmark numbers are still a big improvement?!

[iamdanfox]

Yep, I think we should still see a speedup when lots of requests are queued/in-flight!