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.

Sign up for GitHub

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

Jump to bottom

replace absolute sleep with conditional wait #5971

Merged
merged 1 commit into from
Apr 28, 2015
Merged

replace absolute sleep with conditional wait #5971

merged 1 commit into from
Apr 28, 2015

Conversation

pstratem
Copy link
Contributor

@pstratem pstratem commented Apr 5, 2015

benchmarks pending...

@ajweiss
Copy link
Contributor

ajweiss commented Apr 5, 2015

I like the simpler nature of this patch, but doesn't this mean that that the transaction trickling stuff for privacy turns itself off when the network is sufficiently busy?

@sipa
Copy link
Member

sipa commented Apr 5, 2015 via email

@pstratem
Copy link
Contributor Author

pstratem commented Apr 5, 2015

The trickle logic is essentially completely broken as it is.
It really should be fixed with a seperate patch.

@pstratem
Copy link
Contributor Author

pstratem commented Apr 5, 2015

latency_fix both nodes
2015-04-05 10:55:45 UpdateTip: new best=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048 height=1 log2_work=33.000022 tx=2 date=2009-01-09 02:54:25 progress=0.000000 cache=1
2015-04-05 11:55:36 UpdateTip: new best=000000000000000082ccf8f1557c5d40b21edabb18d2d691cfbf87118bac7254 height=300000 log2_work=78.499549 tx=38463789 date=2014-05-10 06:32:34 progress=0.317754 cache=3218855

latency_fix sync/master ibd node
2015-04-05 22:04:24 UpdateTip: new best=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048 height=1 log2_work=33.000022 tx=2 date=2009-01-09 02:54:25 progress=0.000000 cache=1
2015-04-05 23:16:32 UpdateTip: new best=000000000000000082ccf8f1557c5d40b21edabb18d2d691cfbf87118bac7254 height=300000 log2_work=78.499549 tx=38463789 date=2014-05-10 06:32:34 progress=0.317444 cache=1728437

@ajweiss
Copy link
Contributor

ajweiss commented Apr 5, 2015

Is there any substantial change in CPU load for a busy, listening, full
node?

On Sun, Apr 5, 2015, 6:08 PM Patrick Strateman notifications@github.com
wrote:

with patch on both nodes:
2015-04-05 11:55:36 UpdateTip: new
best=000000000000000082ccf8f1557c5d40b21edabb18d2d691cfbf87118bac7254
height=300000 log2_work=78.499549 tx=38463789 date=2014-05-10 06:32:34
progress=0.317754 cache=3218855
2015-04-05 10:55:45 UpdateTip: new
best=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048
height=1 log2_work=33.000022 tx=2 date=2009-01-09 02:54:25
progress=0.000000 cache=1

—
Reply to this email directly or view it on GitHub
#5971 (comment).

@pstratem
Copy link
Contributor Author

pstratem commented Apr 5, 2015

I haven't measured that, but I cant see there being any substantial cpu overhead.

@pstratem
Copy link
Contributor Author

pstratem commented Apr 6, 2015

latency_fix both nodes: 3591 seconds
2015-04-05 10:55:45 UpdateTip: new best=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048 height=1 log2_work=33.000022 tx=2 date=2009-01-09 02:54:25 progress=0.000000 cache=1
2015-04-05 11:55:36 UpdateTip: new best=000000000000000082ccf8f1557c5d40b21edabb18d2d691cfbf87118bac7254 height=300000 log2_work=78.499549 tx=38463789 date=2014-05-10 06:32:34 progress=0.317754 cache=3218855

latency_fix sync/master ibd node: 4338 seconds
2015-04-05 22:04:24 UpdateTip: new best=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048 height=1 log2_work=33.000022 tx=2 date=2009-01-09 02:54:25 progress=0.000000 cache=1
2015-04-05 23:16:32 UpdateTip: new best=000000000000000082ccf8f1557c5d40b21edabb18d2d691cfbf87118bac7254 height=300000 log2_work=78.499549 tx=38463789 date=2014-05-10 06:32:34 progress=0.317444 cache=1728437

master sync/master ibd: 4952
2015-04-06 00:16:39 UpdateTip: new best=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048 height=1 log2_work=33.000022 tx=2 date=2009-01-09 02:54:25 progress=0.000000 cache=1
2015-04-06 01:39:11 UpdateTip: new best=000000000000000082ccf8f1557c5d40b21edabb18d2d691cfbf87118bac7254 height=300000 log2_work=78.499549 tx=38463789 date=2014-05-10 06:32:34 progress=0.317379 cache=2357152

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 6, 2015

That is about a 28% speedup. Awesome. Care to get started on a separate PR based on this which improves the trickle logic?

@sipa
Copy link
Member

sipa commented Apr 7, 2015

lightly-tested-ACK

@laanwj laanwj added the P2P label Apr 8, 2015
@pstratem pstratem mentioned this pull request Apr 9, 2015
Closed
@ajweiss
Copy link
Contributor

ajweiss commented Apr 15, 2015

I've been playing with this a little bit and it looks pretty good. I ran under a test harness that simulated 125 clients that sent 1-2 messages/sec each (ping/pongs) in order to take a look at changes in the load profile and it does roughly double the overhead, but on my system (2014 era decent consumer PC) I only see about 6% CPU utilization jumping to about 12% so it's probably negligible. (This doubling seems pretty constant even under much higher load, so something to keep in mind for the future.) I'm also playing with running this as a listening node with so-far, so-good results. These sorts of changes tend to scare me a bit as they can have nonobvious network effects, so I'm going to keep playing and will report if I find anything...

@gmaxwell
Copy link
Contributor

We can likely back off the timeout in the future, the current value is probably excessively conservative.

@ajweiss
Copy link
Contributor

ajweiss commented Apr 15, 2015

Wait, wait. It looks like this pull is completely subsumed by #5989? I suppose they need to go in together anyway...

@gmaxwell
Copy link
Contributor

@ajweiss it's just building on it.

@sipa sipa mentioned this pull request Apr 17, 2015
Closed
sipa
sipa reviewed Apr 22, 2015
View reviewed changes
src/net.cpp
@@ -1417,7 +1423,7 @@ void ThreadMessageHandler()
}

if (fSleep)
MilliSleep(100);
messageHandlerCondition.timed_wait(lock, boost::posix_time::microsec_clock::universal_time()+boost::posix_time::milliseconds(100));
Copy link
Member

Choose a reason for hiding this comment

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

Spaces around +

@gavinandresen
Copy link
Contributor

ACK. Code looks good (really tiny nit: messageHandlerCondition could be declared static).
Runs nicely on OSX, still uses ~no CPU when there is nothing to do.

@laanwj
Copy link
Member

laanwj commented Apr 24, 2015

Code changes look good to me, going to test this.

@sipa
Copy link
Member

sipa commented Apr 27, 2015

ACK. Squash please?

@pstratem
Copy link
Contributor Author

Squashed

@laanwj
Copy link
Member

laanwj commented Apr 28, 2015

Tested ACK

@laanwj laanwj merged commit 351593b into bitcoin:master Apr 28, 2015
laanwj added a commit that referenced this pull request Apr 28, 2015
@laanwj
Merge pull request #5971
18d2832 
351593b replace absolute sleep with conditional wait (pstratem)
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pstratem @ajweiss @sipa @gmaxwell @gavinandresen @laanwj