feat: implement read write locks in threading and use them for CActiveMasternodeManager::cs by PastaPastaPasta · Pull Request #5961 · dashpay/dash

PastaPastaPasta · 2024-03-27T17:54:40Z

Issue being fixed or feature implemented

We have some caches or other information in codebase which are read from a lot; but rarely written to. We can use a RW lock here instead of a normal Mutex

What was done?

Implement a RW lock and use them

How Has This Been Tested?

Hasn't been much; looking for review atm. Maybe should deploy this on testnet for a bit and make sure it doesn't break.

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

UdjinM6

pls check be16627

knst · 2024-03-29T15:08:07Z

src/sync.h

 typedef AnnotatedMixin<std::mutex> Mutex;
+/** Wrapped shared mutex: supports read locking via .shared_lock, exlusive locking via .lock;
+ * does not support recursive locking */
+typedef SharedAnnotatedMixin<std::shared_mutex> SharedMutex;


nit: why won't use modern syntax such as:
using SharedMutex = SharedAnnotatedMixin<std::shared_mutex>

knst

LGTM

knst · 2024-03-29T15:09:12Z

src/sync.h

+
+    bool TrySharedEnter(const char* pszName, const char* pszFile, int nLine)
+    {
+        EnterCritical(pszName, pszFile, nLine, Base::mutex(), true);


Suggested change

EnterCritical(pszName, pszFile, nLine, Base::mutex(), true);

EnterCritical(pszName, pszFile, nLine, Base::mutex(), /*fTry = */ true);

knst · 2024-03-29T15:12:43Z

src/sync.h

+    SharedLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) SHARED_LOCK_FUNCTION(pmutexIn)
+    {
+        if (!pmutexIn) return;
+
+        *static_cast<Base*>(this) = Base(*pmutexIn, std::defer_lock);
+        if (fTry) {
+            TrySharedEnter(pszName, pszFile, nLine);
+        } else {
+            SharedEnter(pszName, pszFile, nLine);
+        }
+    }


~~why we need even to consider a situation when pmutexIn can be non-null ptr?~~
ok it seems as copy-paste of UniqueLock

Suggested change

SharedLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) SHARED_LOCK_FUNCTION(pmutexIn)

{

if (!pmutexIn) return;

*static_cast<Base*>(this) = Base(*pmutexIn, std::defer_lock);

if (fTry) {

TrySharedEnter(pszName, pszFile, nLine);

} else {

SharedEnter(pszName, pszFile, nLine);

}

}

knst · 2024-03-29T15:19:27Z

src/evo/mnauth.cpp

+    const bool is_basic_scheme_active{DeploymentActiveAfter(tip, Params().GetConsensus(), Consensus::DEPLOYMENT_V19)};
+    auto pk = ::activeMasternodeManager->GetPubKey();
+    const CBLSPublicKeyVersionWrapper pubKey(pk, !is_basic_scheme_active);
+    uint256 signHash = [&]() {


missing const:

Suggested change

uint256 signHash = [&]() {

const uint256 signHash = [&]() {

knst · 2024-03-30T18:49:24Z

hm, what if there would be too many reading operations which are running in many threads - is there any chance that regular lock ever be acquired? or it would wait until all reading operations are done?

PastaPastaPasta · 2024-04-01T15:45:10Z

hm, what if there would be too many reading operations which are running in many threads - is there any chance that regular lock ever be acquired? or it would wait until all reading operations are done?

From the paper that introduces them

shared_mutex Reference Implementation

shared_mutex can certainly be implemented on top of an OS supplied read-write mutex. However a portable subset of the implementation is shown here for the purpose of motivating the existence of cond_var: the razor-thin layer over the OS condition variable.

A secondary motivation is to explain the lack of reader-writer priority policies in shared_mutex. This is due to an algorithm credited to Alexander Terekhov which lets the OS decide which thread is the next to get the lock without caring whether a unique lock or shared lock is being sought. This results in a complete lack of reader or writer starvation. It is simply fair.

Below is most of the implementation of shared_mutex demonstrating that with this proposal, very high quality synchronization devices can be easily coded, and with the same efficiency as if they were coded straight to the OS. The timed-locking functions are omitted only for brevity. They are similar to the locking functions but make use of cond_var::timed_wait. Dependence on the lower-level C++ API is highlighted.

Based on this I don't think there is an ability for starvation

UdjinM6

utACK

knst

utACK 0692826

, bitcoin#24830, bitcoin#24464, bitcoin#24757, bitcoin#25202, bitcoin#25217, bitcoin#25292, bitcoin#25614, partial bitcoin#22766 (logging backports) 1621696 log: restore `LogPrintLevel` messages from prior backports (Kittywhiskers Van Gogh) 52a1263 merge bitcoin#25614: Severity-based logging, step 2 (Kittywhiskers Van Gogh) 21470fd merge bitcoin#25292: Add LogPrintLevel to lint-format-strings, drop LogPrint-vs-LogPrintf section in dev notes (Kittywhiskers Van Gogh) 026409e merge bitcoin#25217: update lint-logs.py to detect LogPrintLevel, mention WalletLogPrintf (Kittywhiskers Van Gogh) b046e09 merge bitcoin#25202: Use severity-based logging for leveldb/libevent messages, reverse LogPrintLevel order (Kittywhiskers Van Gogh) 7697b73 revert dash#2794: Disable logging of libevent debug messages (Kittywhiskers Van Gogh) ff6304f merge bitcoin#24757: add `DEBUG_LOCKCONTENTION` to `--enable-debug` and CI (Kittywhiskers Van Gogh) 88592f3 merge bitcoin#24464: Add severity level to logs (Kittywhiskers Van Gogh) d3e837a merge bitcoin#24830: Allow -proxy="" setting values (Kittywhiskers Van Gogh) 0e01d5b partial bitcoin#22766: Clarify and disable unused ArgsManager flags (Kittywhiskers Van Gogh) a9cfbd1 fix: don't use non-existent `PrintLockContention` in `SharedEnter` (Kittywhiskers Van Gogh) f331cbe merge bitcoin#24770: Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directive (Kittywhiskers Van Gogh) d9cc2ea merge bitcoin#23104: Avoid breaking single log lines over multiple lines in the log file (Kittywhiskers Van Gogh) 479ae82 merge bitcoin#23235: Reduce unnecessary default logging (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * This pull request's primary purpose is to restore `LogPrintLevel`s from backports in [dash#6333](#6333) that were changed to `LogPrint`s as they were backported before `LogPrintLevel` was backported. * ~~`clang-format` suggestions for `LogPrintLevel` have to be ignored in order to prevent the linter from tripping due to a "missing newline" ([build](https://gitlab.com/dashpay/dash/-/jobs/8398818860#L54)).~~ Resolved by applying diff ([source](#6399 (comment))). * `SharedLock` was introduced in [dash#5961](#5961) and `PrintLockContention` was removed in [dash#6046](#6046) but the changes in the latter were not extended to the former. This has been corrected as part of this pull request. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: f2d0ef8ce5cb1091c714a2169e89deb33fa71ff174ce4e6147b3ad421f57a84183d2a9e76736c0b064b2cc70fb3f2e545c42b8562cf36fdce18c3fb61307c364

UdjinM6 requested changes Mar 28, 2024

View reviewed changes

PastaPastaPasta added 2 commits March 27, 2024 20:41

feat: implement Read Write Locks in threading

663774c

refactor: make CActiveMasternodeManager::cs SharedMutex and private

0692826

PastaPastaPasta force-pushed the new-feat-read branch from 555b4b5 to 0692826 Compare March 28, 2024 01:41

PastaPastaPasta requested a review from UdjinM6 March 28, 2024 16:03

knst reviewed Mar 29, 2024

View reviewed changes

knst approved these changes Mar 29, 2024

View reviewed changes

UdjinM6 approved these changes Apr 2, 2024

View reviewed changes

knst approved these changes Apr 3, 2024

View reviewed changes

PastaPastaPasta added this to the 21 milestone Apr 3, 2024

PastaPastaPasta merged commit dc6f52a into dashpay:develop Apr 3, 2024

PastaPastaPasta deleted the new-feat-read branch April 3, 2024 15:36

kwvg mentioned this pull request Nov 20, 2024

backport: merge bitcoin#23235, #23104, #24770, #24830, #24464, #24757, #25202, #25217, #25292, #25614, partial bitcoin#22766 (logging backports) #6399

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement read write locks in threading and use them for CActiveMasternodeManager::cs#5961

feat: implement read write locks in threading and use them for CActiveMasternodeManager::cs#5961
PastaPastaPasta merged 2 commits intodashpay:developfrom
PastaPastaPasta:new-feat-read

PastaPastaPasta commented Mar 27, 2024 •

edited

Loading

Uh oh!

UdjinM6 left a comment

Uh oh!

knst Mar 29, 2024

Uh oh!

knst left a comment

Uh oh!

knst Mar 29, 2024

Uh oh!

knst Mar 29, 2024

Uh oh!

knst Mar 29, 2024

Uh oh!

knst commented Mar 30, 2024

Uh oh!

PastaPastaPasta commented Apr 1, 2024

Uh oh!

UdjinM6 left a comment

Uh oh!

knst left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	EnterCritical(pszName, pszFile, nLine, Base::mutex(), true);
	EnterCritical(pszName, pszFile, nLine, Base::mutex(), /fTry = / true);

Conversation

PastaPastaPasta commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

knst Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

knst Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

knst Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

knst commented Mar 30, 2024

Uh oh!

PastaPastaPasta commented Apr 1, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PastaPastaPasta commented Mar 27, 2024 •

edited

Loading