Disclaimer: I'm not familiar with asyncore, so I'm unclear how safe this is. It works for me (tm).
Because the poll/select loop may pause for 100msec before actually doing a send, and we have no way to force the loop awake, try sending from the calling thread if the queue is empty.
Also, disable nagle as all sends should be either full messages or unfinished sends.
This shaves an average of ~1 minute or so off of my accumulated runtime, and 10-15 seconds off of actual runtime.
Because the poll/select loop may pause for 100msec before actually doing a
send, and we have no way to force the loop awake, try sending from the calling
thread if the queue is empty.
Also, disable nagle as all sends should be either full messages or unfinished
sends.
This shaves an average of ~1 minute or so off of my accumulated runtime, and
10-15 seconds off of actual runtime.
Mega concept ACK!
Tested ACK 1817398b397afebcc857c40a16d201c84878cb89. I've run the extended test suite over this locally and it passes. It makes any test using the p2p interface much faster.
Travis has intermittent timing issues, so we'd need to make sure this is well tested in that environment before merging.
1792 | @@ -1792,7 +1793,14 @@ def send_message(self, message, pushbuf=False): 1793 | tmsg += h[:4] 1794 | tmsg += data 1795 | with mininode_lock: 1796 | - self.sendbuf += tmsg 1797 | + if (len(self.sendbuf) == 0 and not pushbuf):
nit: if not self.sendbuf and not pushbuf:. From PEP8: "For sequences, (strings, lists, tuples), use the fact that empty sequences are false."
No need to fix this "style nit". len(some_list)==0 is stricter than just casting any object/primitive type to boolean. Also it is more clear to read, imo.
@MarcoFalke I believe that for values of the list type if len(some_list)==0 is actually identical to if not some_list, and in general I'd argue for following things like PEP8.
ACK with or without this nit change. I have a slight preference for being pythonic and following PEP8. Leaving the longer form (if (len(self.sendbuf) ==0 would catch any bugs where someone set self.sendbuf to something other than a list, but I don't think that's a good justification.
Either way I think the brackets should at least be removed
@sipa, it is identical. Though, in the case that some_list is False or None or 0 or anything else that casts to False, len(some_list) will fail appropriately. I don't think it is a downside to fail in the test framework when a parameter has the wrong type.
I think to be hyper pedantic, if len(self.sendbuf) == 0 is equivalent to:
if not hasattr(self.sendbuf, '__len__'):
raise TypeError
else if not self.sendbuf:
# do stuff
ie, it basically has an implicit assert that the type of self.sendbuf has a length property.
In general we don't litter the Python code with type checks, even though it's not a type safe language and someone could call into functions with the wrong typed parameters.
👍 on @jimpo and @MeshCollider suggestions.
Cory is being modest as usual. Let's run this over just the tests that use the p2p network thread.
master:
→ ./test_runner.py $(grep -l NetworkThread *py | tr '\n' ' ')
Temporary test directory at /tmp/user/1000/bitcoin_test_runner_20170915_153854
.............
TEST | STATUS | DURATION
assumevalid.py | ✓ Passed | 12 s
bip65-cltv-p2p.py | ✓ Passed | 7 s
bip68-112-113-p2p.py | ✓ Passed | 21 s
bip9-softforks.py | ✓ Passed | 49 s
bipdersig-p2p.py | ✓ Passed | 6 s
example_test.py | ✓ Passed | 3 s
invalidblockrequest.py | ✓ Passed | 4 s
invalidtxrequest.py | ✓ Passed | 3 s
maxuploadtarget.py | ✓ Passed | 149 s
nulldummy.py | ✓ Passed | 3 s
p2p-acceptblock.py | ✓ Passed | 6 s
p2p-compactblocks.py | ✓ Passed | 29 s
p2p-feefilter.py | ✓ Passed | 32 s
p2p-fullblocktest.py | ✓ Passed | 108 s
p2p-leaktests.py | ✓ Passed | 8 s
p2p-mempool.py | ✓ Passed | 3 s
p2p-segwit.py | ✓ Passed | 67 s
p2p-timeouts.py | ✓ Passed | 65 s
p2p-versionbits-warning.py | ✓ Passed | 8 s
sendheaders.py | ✓ Passed | 24 s
ALL | ✓ Passed | 607 s (accumulated)
Runtime: 166 s
this branch:
TEST | STATUS | DURATION
assumevalid.py | ✓ Passed | 12 s
bip65-cltv-p2p.py | ✓ Passed | 7 s
bip68-112-113-p2p.py | ✓ Passed | 13 s
bip9-softforks.py | ✓ Passed | 52 s
bipdersig-p2p.py | ✓ Passed | 6 s
example_test.py | ✓ Passed | 3 s
invalidblockrequest.py | ✓ Passed | 3 s
invalidtxrequest.py | ✓ Passed | 3 s
maxuploadtarget.py | ✓ Passed | 64 s
nulldummy.py | ✓ Passed | 3 s
p2p-acceptblock.py | ✓ Passed | 6 s
p2p-compactblocks.py | ✓ Passed | 20 s
p2p-feefilter.py | ✓ Passed | 21 s
p2p-fullblocktest.py | ✓ Passed | 99 s
p2p-leaktests.py | ✓ Passed | 8 s
p2p-mempool.py | ✓ Passed | 3 s
p2p-segwit.py | ✓ Passed | 51 s
p2p-timeouts.py | ✓ Passed | 64 s
p2p-versionbits-warning.py | ✓ Passed | 8 s
sendheaders.py | ✓ Passed | 14 s
ALL | ✓ Passed | 460 s (accumulated)
Runtime: 139 s
That's a ~25% reduction in cumulative test time, with particularly impressive reduction in maxuploadtarget.py
utACK 1817398b397afebcc857c40a16d201c84878cb89, wanna fix @jimpo's style nit?
utACK
Nice, Concept ACK
I'll defer on the style nit, I'm not familiar enough with python to have a preference.
Nice! utACK 1817398b397afebcc857c40a16d201c84878cb89
Concept ACK
utACK 1817398b397afebcc857c40a16d201c84878cb89. Not going to hold this up on style nits, as there are other places where this is "wrong" and we don't enforce any style in the python code as of now.