CommandQueueMT: Optimize & fix handling of sync/ret commands #90760
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on changes to threaded servers, I realized this class lacked a lock-unlock before marking a sync semaphore as unused. The bug itself would be solved just by locking there.
However, I also noticed that the approach of having a pool of semaphores could be replaced by one based on a condition variable and sequence numbering. Besides simplifying the code, this reduces the duplication of mutexes to lock-unlock (the main in the command queue class and the one in the sempahore) and avoids the spinning wait. Since both have overlapping concerns, we can go one level down, that is, splitting the semaphore into its building blocks (mutex and cond var) and letting our waits-for-sync be based on such cond var.
The only downside is that all threads waiting for syncing on commands will be awaken. Nonetheless, I think that's a no-issue in practice since normally you won't have multiple threads trying to sync on different commands. And, anyway, if a thread waiting to sync just awakes it can quickly check the command its interested in hasn't come yet and go back to waiting.
TL;DR
Pros:
Cons: