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

Allow SendMessages to run partially without cs_main #2840

Merged
merged 3 commits into from
Oct 21, 2013

Conversation

sipa
Copy link
Member

@sipa sipa commented Jul 20, 2013

SendMessages() tries to acquire a cs_main lock now, but this isn't nessecary for much of its functionality. Move those parts out of the if condition.

@sipa
Copy link
Member Author

sipa commented Jul 25, 2013

Added a few more lock improvements.

@sipa
Copy link
Member Author

sipa commented Aug 15, 2013

Rebased.

@Diapolo
Copy link

Diapolo commented Aug 17, 2013

Does this make the code more stable or is it a performance-improving patch? That locking stuff isn't my main expertise ^^.

@sipa
Copy link
Member Author

sipa commented Aug 18, 2013

@Diapolo It improves performance/throughput, by allowing more code to run without needing the cs_main lock, and conversely, holding that lock for a shorter time, so more other code can run in parallel.

At this point the improvements may not be noticable, but if we ever want a decently parallellizable core, we can't avoid doing this everywhere it's possible.

@sipa
Copy link
Member Author

sipa commented Oct 14, 2013

Rebased.

PS: the first commit may look large, but it really just changes some indentation. If needed, I can make a version that keeps the indentation identical for now.

if (!AlreadyHave(inv))
{
if (fDebugNet)
printf("sending getdata: %s\n", inv.ToString().c_str());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be LogPrint("net", ...)

@gavinandresen
Copy link
Contributor

Looks good to me except for an errant printf (reviewed using a graphical diff tool-- opendiff on the mac-- using its "compress whitespace" preference).

@sipa
Copy link
Member Author

sipa commented Oct 15, 2013

Modified to not do indentation changes for now.

SendMessages() tries to acquire a cs_main lock now, but this isn't nessecary
for much of its functionality. Move those parts out of the locked section,
so they can always be performed, and we hold cs_main for a shorter time.
@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7d38af3c493f9ea24c722ec2e6d3c51f4e851364 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

gavinandresen added a commit that referenced this pull request Oct 21, 2013
Allow SendMessages to run partially without cs_main
@gavinandresen gavinandresen merged commit 749230d into bitcoin:master Oct 21, 2013
IntegralTeam pushed a commit to IntegralTeam/bitcoin that referenced this pull request Jun 4, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants