fixes in async_semaphore for concurrency ID calculation

[gshuflin]

Problem

This is an attempt to fix an issue where the concurrency ID passed into processes executed under the BoundedCommandRunner is being limited to a smaller set of IDs than the limit of the BoundedCommandRunner.

We were previously using the (usize) value of available_permits on the inner, locked portion of the AsyncSemaphor data structure that BoundedCommandRunner uses internally, as the value for the concurrency ID. However, this resulted in undesirable behavior where IDs passed to processes would count down from the local concurrency limit value (e.g. 16) to 1, and then never rise above 1 again. Every time a task finished, it would increment available_permits from 0 to 1, and then wake up the next process waiting on the queue, which would decrement available_permits again when it began to run and use 1 as its ID.

There was also an issue with always waking up the task in the front position in the queue. In the case where a large number of processes were all scheduled at once, they might all call wake_by_id on the same waiting task, meaning that any other waiting tasks after the first would wouldn't get awoken until either the first task, or a newly-scheduled task, completed and woke up tasks in the queue beyond the first.

Solution

To solve the concurrency ID issue, we add a new queue data structure to Inner, that keeps track of discrete IDs. When a permit to run is issued (in PermitFuture::poll), an ID is popped from the front of this queue, and when a task completes (in Permit::drop) that ID is pushed onto the back of the queue. So, a limited number of IDs will always be cycling through the AsyncSemaphor as long as it is running, and we won't see the behavior. This change also has the effect of causing IDs to count up from 1 rather than down from the local concurrency limit, which is only an implementation detail, since these IDs are meant to be opaque identifiers.

When an asynchronous task completes, instead of calling wake_by_ref() on the waiter at the front of the queue, use available_permits to index into the waiters queue and wake up a task. This ensures that each completing task will wake up at least one yet-unwoken waiting task (if there are any such waiting tasks), which will cause those waiting tasks to get scheduled immediately and take up a concurrency slot.

Also, a debug logger with the concurrency ID is added in this commit, to facilitate debugging any further concurrency-ID-related issues we may run into.

[coveralls]

Coverage remained the same at 0.0% when pulling 0d623e9 on gshuflin:fixing_concurrency_id into ef79642 on pantsbuild:master.

[gshuflin]

As an illustration of the changes this patch makes, here is some log output from pants running ./pants --no-pantsd --local-execution-root-dir=/home/gregs/large-partition/ test src/python/pants:: on my system with "info" level logging of the concurrency ID:

Old:
old-code.log

New:
newer-code.log

[stuhood]

Thanks!

There was also an issue with always waking up the task in the front position in the queue. In the case where a large number of processes were all scheduled at once, they might all call wake_by_id on the same waiting task, meaning that any other waiting tasks after the first would wouldn't get awoken until either the first task, or a newly-scheduled task, completed and woke up tasks in the queue beyond the first.

Good fix. But IMO, we should really, really switch to tokio semaphore. I'm fine with not doing it in this PR as long as you're confident in the tests.

[stuhood]

It would probably be less error prone for this to either:

1. replace the inner.available_permits counter entirely, so that we can be sure that we don't promise a permit unless there is an available_id.
2. be decoupled from the semaphore and completely independent (such that it generated tokens lazily if it was empty, counting upward or something).

But if you trust that the test is sufficient, it's not a blocker per-se.