replace absolute sleep with conditional wait

pstratem commented at 9:38 am on April 5, 2015: contributor

benchmarks pending…

ajweiss commented at 5:15 pm on April 5, 2015: contributor

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 commented at 6:03 pm on April 5, 2015: member

Agree. If we want the trickling behaviour still, we should make it conditional on sufficient time having elapsed?

pstratem commented at 10:00 pm on April 5, 2015: contributor

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

pstratem commented at 10:07 pm on April 5, 2015: contributor

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 commented at 10:12 pm on April 5, 2015: contributor

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 commented at 10:26 pm on April 5, 2015: contributor

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

pstratem commented at 2:29 am on April 6, 2015: contributor

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 commented at 2:54 am on April 6, 2015: contributor

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

sipa commented at 10:15 pm on April 7, 2015: member

lightly-tested-ACK

laanwj added the label P2P on Apr 8, 2015

ajweiss commented at 5:30 pm on April 15, 2015: contributor

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 commented at 5:35 pm on April 15, 2015: contributor

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

ajweiss commented at 5:41 pm on April 15, 2015: contributor

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

gmaxwell commented at 5:45 pm on April 15, 2015: contributor

@ajweiss it’s just building on it.

in src/net.cpp: in e758e8d879 outdated

1422@@ -1417,7 +1423,7 @@ void ThreadMessageHandler()
1423         }
1424 
1425         if (fSleep)
1426-            MilliSleep(100);
1427+            messageHandlerCondition.timed_wait(lock, boost::posix_time::microsec_clock::universal_time()+boost::posix_time::milliseconds(100));

sipa commented at 4:52 pm on April 22, 2015:

Spaces around +

gavinandresen commented at 2:59 pm on April 24, 2015: 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 commented at 4:23 pm on April 24, 2015: member

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

sipa commented at 12:44 pm on April 27, 2015: member

ACK. Squash please?

pstratem commented at 6:46 pm on April 27, 2015: contributor

Squashed

replace absolute sleep with conditional wait 351593b9c8

laanwj commented at 8:40 am on April 28, 2015: member

Tested ACK

laanwj merged this on Apr 28, 2015

laanwj closed this on Apr 28, 2015

laanwj referenced this in commit 18d2832678 on Apr 28, 2015

DrahtBot locked this on Sep 8, 2021

replace absolute sleep with conditional wait #5971