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
remove useless millisleep #4790
Conversation
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) |
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. |
reduces time to service requests improving performance
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4790_9189f5fe4df1ac7ea6ca75ceada867beafda90a9/ for binaries and test log. |
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. |
9189f5f remove useless millisleep (phantomcircuit)
reduces time to service requests improving performance Rebased-from: 9189f5fe4df1ac7ea6ca75ceada867beafda90a9 bitcoin/bitcoin#4790
reduces time to service requests improving performance
calls is unnecessary as select() blocks and on select() failure there is another MilliSleep()