Thread pool - Fix a couple of things that were reverted

[kouvel]

Reverting PR: dotnet/coreclr#14015

• Fix multiple thread requests per enqueued item (original change: 99db31c)
• Fix excessive attempts to steal from other local queues (original change: fd91ee1)

[kouvel]

I believe reducing the number of thread requests is going to require some more interlocked operations on the main paths (at least one more per enqueue, at least one more per dequeue) that will (as of my last testing) reduce throughput a noticeable amount from the current state.

I'm actually not convinced that interlocked operations are not necessary now. Though issuing a thread request on enqueue and dequeue as done currently would significantly reduce the probability of failure, it does not look correct nonetheless.

For instance, an enqueuer that reads an old value for the thread request count and seeing that it is equal to the processor count, may miss a thread request if it read the wrong value of the thread request count. This potential for error would be significantly compensated by ensuring a thread request on every successful dequeue, but I don't think it provides complete compensation such that the current code would be correct.

Potential case:

• Proc count: 4
• Initial state: 4 pending thread requests, 4 work items in queue, all worker threads waiting
• 4 worker threads wake and go into ThreadPool.Dispatch()
  • Current state: 0 pending thread requests
• Those 4 worker threads dequeue 4 work items and request 4 threads
  • Current state: 4 pending thread requests
• Those 4 worker threads determine that there is no work available, exit ThreadPool.Dispatch(), and come back to ThreadPool.Dispatch() because of the pending thread requests
  • Current state: 0 pending thread requests
• Another thread enqueues a work item and incorrectly sees 4 pending thread requests (I don't see why this is not possible), skipping the thread request
• Now there's a work item in the queue but no thread request. This would only be an issue if no more work items are enqueued thereafter.

Similarly, a dequeuer may miss a thread request because it sees an incorrect value for the pending thread request count, preventing the starvation heuristic from kicking in due to a lack of thread requests while there are work items in the queue. Both issues would mostly go unnoticed because typically more work items would be queued soon enough.

@stephentoub, thoughts?

I believe some compromise in perf can be had by avoiding p/invokes and at this point that's far in the future (a reasonable direction would involve porting CoreRT thread pool to CoreCLR). Moving to future. I'll follow up on this again once I have time to investigate further.

[sdmaclea]

@benaadams @kouvel
One other approach I imagined was using the global queue to trigger cross thread polls. I have not had time to prototype it. It is something I will look at when I get back to optimizing plaintext.

[benaadams]

So ThreadPoolWorkQueue.EnsureThreadRequested periodically shows up in my traces as one of the hottest functions (for own time)

I originally believed it to be contention on the CAS of numOutstandingThreadRequests; as it has multiple points to be over called.

Looking at the ratio of calls between EnsureThreadRequested and MarkThreadRequestSatisfied which use the same variable; they are fairly close in call count; however EnsureThreadRequested's own time is much more disproportionate.

So its more likely to be the native call ThreadPool.RequestWorkerThread() that's taking the time?

[kouvel]

EnsureThreadRequested is called for each queued work item so typically it would be called more frequently than MarkThreadRequestSatisfied. If the call counts are similar between those, it must be bursty work where the queue frequently reaches zero or short lengths. I'm seeing that in bursty cases most of the time taken in EnsureThreadRequested is in under RequestWorkerThread, this is expected because threads will be running out of work frequently. In steady-state cases where the queue doesn't deplete, most of the time taken under EnsureThreadRequested is in reading the static variable processorCount (which could probably be improved).

[benaadams]

As a specific example, in the techempower tests:

For plaintext (the ultra high-throughput test) EnsureThreadRequested doesn't show up as significant.

For the other tests, which behave more in a ping-pong, request->response->request->response scenario it starts showing up as a factor; these I think are more bursty, which fits your explanation.

Equally ASP.NET Core does very well on plaintext; but looses some ground on the other tests, so the pause->burst cycle does seem to be hurting it.

Not sure why at this is currently; whether bursts of queuing on the thread pool or resuming many task statemachines.