Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: convert m_nodes_mutex to a non-recursive mutex #6473

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

If we are able to get this merged, I'll probably try to upstream this.

m_nodes_mutex is heavily contended; likely more-so in dash than bitcoin, due to master nodes aggressively sending out signature shares and related messages, and additional logic that uses stuff like ForEachNode.

Long term, I may want to convert the non-recursive mutex into a shared mutex, similar to the logic in 6468 as m_nodes is only actually mutated (hence needing an exclusive lock) when nodes are added or removed. CNode is internally thread safe.

As you can see below, m_nodes_mutex represents ~90% of all contention on ~latest nightly on testnet during tx/islock spamming. Making this into a non-recursive mutex will likely significantly reduce the time a contention takes, although, the contention itself likely won't go away.

image

What was done?

Convert m_nodes_mutex from RecursiveMutex -> Mutex and fix all compilation errors.

How Has This Been Tested?

This should be tested with a tsan reindex and otherwise tsan test suite, which has not been done yet.

Breaking Changes

N/a

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)

@PastaPastaPasta PastaPastaPasta added this to the 22.1 milestone Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

This pull request has conflicts, please rebase.

@PastaPastaPasta PastaPastaPasta force-pushed the perf-m_nodes_mutex-non-recursive branch from 32a7a8c to fb561ef Compare December 9, 2024 21:22
@UdjinM6
Copy link

UdjinM6 commented Dec 11, 2024

CI is pretty unhappy...

@PastaPastaPasta
Copy link
Member Author

I had changes that fixed this, but they're on my dead system... will have to wait a week or so before I can recover them.

@PastaPastaPasta PastaPastaPasta marked this pull request as draft December 15, 2024 18:13
Copy link

This pull request has conflicts, please rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants