[DRAFT] Add ReadWrite locking mechanism on top of Blockchain. #9173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xFFFC0000
force-pushed
the
dev/0xfffc/rw-lock-blockchain
branch
3 times, most recently
from
73e87fe
to
27f98a7
Compare
jeffro256
reviewed
Feb 15, 2024
src/cryptonote_core/blockchain.cpp
Outdated
Comment on lines
1352
to
1416
| m_blockchain_transaction.start_write(); | ||
| epee::misc_utils::auto_scope_leave_caller scope_exit_handler = | ||
| epee::misc_utils::create_scope_leave_handler([&](){ | ||
| m_blockchain_transaction.end_write(); | ||
| }); |
There was a problem hiding this comment.
I feel like this pattern is a good candidate for an RAII class. This class could also let you lock and unlock more finely within this scope depending on the operations required.
There was a problem hiding this comment.
I disagree here. Eventually, we want to have a fine-grained locking mechanism. RAII here would just make this a lot more cumbersome to use.
Consider this as prototype/experimental PR, which does have bugs, that is why I am using auto_scope_leave_caller here. Eventually, we want to only lock where we interact with write/reads. Not whole functions.
There was a problem hiding this comment.
You can still have fine-grained locking with RAII. Consider std::unique_lock.
src/cryptonote_core/blockchain.cpp
Outdated
| @@ -1102,7 +1136,11 @@ size_t Blockchain::recalculate_difficulties(boost::optional<uint64_t> start_heig | |||
| //------------------------------------------------------------------ | |||
| std::vector<time_t> Blockchain::get_last_block_timestamps(unsigned int blocks) const | |||
| { | |||
| CRITICAL_REGION_LOCAL(m_blockchain_lock); | |||
| m_blockchain_transaction.start_write(); | |||
There was a problem hiding this comment.
Why are all these start_write() even for functions which only read data?
There was a problem hiding this comment.
Consider this as prototype/experimental PR, which does have bugs.
This is a symptom of a bigger problem we have. Imagine this scenario: Functions calls: A->B->C->D.
A is a write transaction. But B, C and D are read transactions. We have to acquire a write lock for B, C, and D since read and write transactions in this implementation are exclusive. (I am fixing it).
Piggybacking on the previous comment, if we do implement more fine-grained locking we would not have this problem.
src/cryptonote_core/blockchain.h
Outdated
| boost::condition_variable_any r_condition; | ||
|
|
||
| boost::recursive_mutex w_mutex; | ||
| std::atomic<unsigned int> w_locks; |
There was a problem hiding this comment.
Why would there allowed to be more than 1 write lock at a time?
There was a problem hiding this comment.
Recursive function calls.
E.g. Imagine this function calls: A->B->C->D.
A, B, and C are a write transaction. But D is read transactions.
(this will be fixed too).
One other approach would just check boost::thread::id and if you are the owner then don't lock again. That's one approach. I am evaluating which way we should go.
There was a problem hiding this comment.
How much would the code have to be refactored so that we don't use the locks recursively in the first place? (probably a lot)
There was a problem hiding this comment.
That is one of the cleaner approaches and end result of that approach would be much much cleaner. But the problem with that is a lot of APIs should change.
There was a problem hiding this comment.
take a look at this line:
This is much cleaner. But at the moment, it is not possible since we have to change a lot of APIs.
Eventually, we want that. A transaction-based blockchain.cpp. Transaction objects will be trivial to create and you can create ReadTransaction and WriteTransaction. For now, this is an intermediary patch to improve.
|
@jeffro256 thanks for your comment. I left some comments too. |
0xFFFC0000
force-pushed
the
dev/0xfffc/rw-lock-blockchain
branch
6 times, most recently
from
2ebdee1
to
cd40269
Compare
|
Leaving this here just to keep in mind. This deadlock should be fixed: Thread 1, waiting while holding the lock: Thread 2, locking while other thread has the lock: |
0xFFFC0000
force-pushed
the
dev/0xfffc/rw-lock-blockchain
branch
13 times, most recently
from
7bbb3ed
to
a52f339
Compare
This will fix existing locking issues, and provide a foundation to implement more fine-grained locking mechanisms in future works.
0xFFFC0000
force-pushed
the
dev/0xfffc/rw-lock-blockchain
branch
from
a52f339
to
70ce45a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This will fix existing locking issues, and provide a foundation to implement more fine-grained locking mechanisms in future works.