godot icon indicating copy to clipboard operation
godot copied to clipboard

CommandQueueMT: Optimize & fix handling of sync/ret commands

Open RandomShaper opened this issue 1 year ago • 0 comments

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:

  • Race condition fixed.
  • Simpler code.
  • No redundant lock-unlock.
  • No limit on awaiters (spinning wait gone).

Cons:

  • Awaiters of a given command are spuriously awaken whenever any other sync command is executed (an actual issue in practice?).

RandomShaper avatar Apr 16 '24 15:04 RandomShaper