remove useless millisleep #4790

reduces time to service requests improving performance

calls is unnecessary as select() blocks and on select() failure there is another MilliSleep()

This looks like a typical fail-safe for avoiding pegging the cpu in a tight loop. Are you sure there are no cases where the the thread ends up looping without a stall? There are plenty of cases where a select can return immediately without error.

@theuni This handles all cases.

timeout - essentially MilliSleep(50) error - MilliSleep(50) fd's ready - we dont want to sleep!

Untested ACK

@pstratem No problem. I didn't mean to imply that it really is needed, only that it may have a non-obvious use.

@theuni I didn't take offense, sanity checking even the most trivial looking changes makes perfect sense.

For those following at home. There are currently two sleeps in the network handling code— one in the thread with the select. One in the threadmessagehandler. The latter is necessary (or at least must be replaced with a semaphor) to prevent a busy loop. This one is not— it should be superfluous to the select, except— perhaps— if there are no connections.

(Posix select is defined to sleep when empty, but someone might want to double check on windows.)

should behave correctly with no connections, select() should sleep for the timeout period (select manpage specifically references calling select like this to get a high precision sleep function).

tested on debian 7.6, ./bitcoind -connect=10.1.1.1 does not result in a busy loop

Yah, that's fine on *nix. Windows is the key platform to test, for select() behavior. it is already quite odd on windows, only working for some types of handles (sockets). No-socket behavior is precisely the area windows would be annoying ;p

@jgarzik yes windows immediately returns WSAEINVAL if the three sets are empty

This triggers the MilliSleep in the error handler

There is only an issue if select returns a value other than -1 without blocking.

That doesn't appear to happen on any platform.

Okay, seems (according to the interwebs) that— indeed— windows returns an error instead of sleeping in the no socket case. But we already have a sleep in the error handling path.

I've tested this on Linux with and without connections. ACK on the change— But I'd like to hear that this was tested with and without any connections (e.g. -connect=0.0.0.0, and check if Bitcoin is pegging the cpu) on Windows and Mac.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4790_9189f5fe4df1ac7ea6ca75ceada867beafda90a9/ 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.

Never knew that -connect=0.0.0.0 could be used to not connect to any nodes. Having a way to not connect was one of my reasons for being in favor of #4687.

ACK, tested on OSX with -connect=0.0.0.0

@pstratem Can you get this crazy merge commit out of here? (just rewrite against master and force push) this could be fixed on merge too but it might safe someone a few minutes to fix it yourself.

@gmaxwell i did... last night..?

@pstratem Weird, github must have been displaying stale data for me.

@gmaxwell I've found their caching to be consistently overly aggressive.