Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts() #24197

1. 
   jonatack commented at 8:02 pm on January 28, 2022: member

   Following up on #22932 (review) by Marco Falke (good observation, thank you), we can replace a cs_main lock in CBlockTreeDB::LoadBlockIndexGuts() with a Clang thread safety annotation/assertion instead. The unlocked code is reverted to its original state before #22932.
2. Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts() 20276ca5d1
3. hebasto approved
4. 
   hebasto commented at 9:18 pm on January 28, 2022: member

   ACK 20276ca5d124285bdd1bda4cd777ca186b378555, I have reviewed the code and it looks OK, I agree it can be merged.
5. DrahtBot added the label UTXO Db and Indexes on Jan 28, 2022
6. PastaPastaPasta approved
7. 
   PastaPastaPasta commented at 4:47 am on January 30, 2022: contributor

   utACK, I reviewed the code; I agree this is safe, and can be merged.

   From a practical perspective, I wonder if this change (or changes like it) would see a performance gain, or if the goal is more about clean code. I also wonder if it is will result in future changes that are sub-optimal. What are the odds that something will want to call this function in the future that is not already locking cs_main?

8. 
   jonatack commented at 4:06 pm on January 30, 2022: member

   Thanks for reviewing!

   From a practical perspective, I wonder if this change (or changes like it) would see a performance gain, or if the goal is more about clean code.

   I think the goal is to remove any unneeded locks for performance, while using locks where needed for thread safety. If you launch bitcoind with debug=lock (or run bitcoin-cli logging [] '["lock"]' on a bitcoind instance) you’ll see how much time is spent in lock contention, and in some cases it is significant. Even in faster or non-hotspot cases, taking a lock, if contended, isn’t particularly cheap so it seems best to not do so needlessly.

   I also wonder if it is will result in future changes that are sub-optimal. What are the odds that something will want to call this function in the future that is not already locking cs_main?

   In this case, thanks to the thread safety annotation the compiler would give a warning, and the run-time assertion may be hit, both of which would serve to warn that a missing lock is needed. At this time, however, LoadBlockIndexGuts() only has one caller in the codebase.

   More information about Clang thread safety analysis, if helpful: - https://clang.llvm.org/docs/ThreadSafetyAnalysis.html - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

9. 
   PastaPastaPasta commented at 7:22 am on January 31, 2022: contributor

   Thanks for the response @jonatack completely agree with minimizing the time spent in critical sections (and doing so saftely)

   I have a few questions if you’d be so kind :)

   How expensive is locking a RecursiveMutex, if the same thread already has acquired it? Is it the same cost as acquiring an unlocked recursive mutex, or basically a no-op?

   My concern is not a future caller would trigger a data-race / thread insafety, the annotations / assertions prevent that. My question is more along the line of, if we introduce a call in the future in some function WITH_LOCK(::cs_main, LoadBlockIndexGuts...); then the scope of the cs_main lock will be broader than needed correct? IE, inside of LoadBlockIndexGuts, only four lines required the cs_main lock, but we were forced to lock cs_main for the entire function call. Does that make sense?

   Thanks

10. 
    MarcoFalke commented at 7:41 am on January 31, 2022: member

    This is only called once in init, which already holds cs_main. It doesn’t really make sense to call this anywhere in a hot loop
11. MarcoFalke merged this on Jan 31, 2022
12. MarcoFalke closed this on Jan 31, 2022

    ---

13. jonatack deleted the branch on Jan 31, 2022
14. 
    jonatack commented at 12:45 pm on January 31, 2022: member

    @PastaPastaPasta if you’d like to dive deeper, have a look at src/sync.{h,cpp}, https://preshing.com/20111118/locks-arent-slow-lock-contention-is/, and maybe the other links in #22736 (comment). I updated my reply above to mention contention. There are also system calls, memory/cache coherency and fences, and wait queues, for example, in addition to lock contentions.
15. sidhujag referenced this in commit 776e028f39 on Feb 1, 2022
16. Fabcien referenced this in commit dcac626d18 on Jan 24, 2023
17. DrahtBot locked this on Jan 31, 2023

Contributors
jonatack hebasto PastaPastaPasta MarcoFalke

Labels
UTXO Db and Indexes