PRs #22736, #22904 and #23223 changed lock contention logging from a DEBUG_LOCKCONTENTION compile-time preprocessor directive to a runtime lock log category and improved the logging output. This changed the locking from using lock() to try_lock():
-
void Mutex::UniqueLock::lock()acquires the mutex and blocks until it gains access to it -
bool Mutex::UniqueLock::try_lock()doesn’t block but instead immediately returns whether it acquired the mutex; it may be used bylock()internally as part of the deadlock-avoidance algorithm
In theory the cost of try_lock might be essentially the same relative to lock. The test-and-set logic of these calls is purported to be ~ constant time, optimised and light/quick if used carefully (i.e. no mutex convoying), compared to system calls, memory/cache coherency and fences, wait queues, and (particularly) lock contentions. See the discussion around #22736 (comment) and after with respect to performance/cost aspects. However, there are reasonable concerns (see here and here) that Base::try_lock() may be potentially costly or risky compared to Base::lock() in this very frequently called code.
One alternative to keep the run-time lock logging would be to gate the try_lock call behind the logging conditional, for example as proposed in https://github.com/bitcoin/bitcoin/commit/ccd73de1dd969097d34634c2be2fc32b03fbd09e and ACKed here. However, this would add the cost of if (LogAcceptCategory(BCLog::LOCK)) to the hotspot, instead of replacing lock with try_lock, for the most frequent happy path (non-contention).
It turns out we can keep the advantages of the runtime lock contention logging (the ability to turn it on/off at runtime) while out of prudence putting the try_lock() call and lock logging category behind a DEBUG_LOCKCONTENTION compile-time preprocessor directive, and also still retain the lock logging enhancements of the mentioned PRs, as suggested in #24734 (comment) by W. J. van der Laan, in #22736 (review), and in the linked IRC discussion.
Proposed here and for backport to v23.
