Rework the clients thread priority handling

[Diapolo]

This is just a suggestion to assist in re-thinking our current thread priorities and assists (IMHO) in easily setting priorities for threads during the creation time of the thread.

I chose to change some default thread priorities, which should also be considered to be part of a discussion. I'm currently using that code and can verify the internal miner did quite happily find new blocks ;).

That pull could be extended to give users the ability to set the default thread prio via command-line or GUI option.

• removes SetThreadPriority() and integrates that into NewThread() with a
  default of THREAD_PRIORITY_NORMAL
• removes special-casing (priority switching) for internal Bitcoin miner
• uses a new default for the following threads: ThreadScriptCheck (below
  normal - because normal prio threads on every CPU core could slowdown UX),
  ThreadImport (above normal - to speed it up a little) and
  ThreadBitcoinMiner (below normal - to compensate the removed special
  casing)
• removes thread priority code for non-Windows OSes, so these will just
  get a no-op

[gavinandresen]

NACK. Setting thread priorities is non-portable; the only reason the code did it originally was to de-prioritize the mining threads. Since we don't really support CPU mining any more, that reason has gone away.

A pull to remove setting thread priorities entirely would be better.

[sipa]

@gavinandresen If this is true: https://bitcointalk.org/index.php?topic=137680.0 it perhaps does make sense to define our own enum with priorities in util.h, and have CreateThread take such an enum value. It could be a no-op in all but WIN32.

[Diapolo]

@gavinandresen I think your point is valid in terms of CPU mining beeing unsupported (although I love to use it on testnet to quickly generate a block), but as @sipa pointed out there is that "issue" in Windows, that the whole OS is lagging, when the ThreadScriptCheck is run. Also there is ThreadMessageHandler2, which uses a non-default priority on Windows... dunno why, but that is the current state ;).

[laanwj]

If we're going to keep thread priorities I agree with this solution to pass the priority at thread creation. There is never a need to change it on the fly and I'm happy to get rid of that inlined function in util.h.

[laanwj]

It is possible on Linux but you need to mess with thread scheduling policies and whatnot, so I recommend leaving it a NOP for everything but windows (but this comment can go, as it is not "unclear" in any way :-)

[Diapolo]

An update is on the way ;).

[Diapolo]

Added:

• removes thread priority code for non-Windows OSes, so these will just
  get a no-op

Still, I think it makes sense to evaluate if the current prios are chosen wisely or if there is room for improvement.

@gavinandresen Are you still on NACK or does it now seem to make sense for Windows :)?

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f2cb7f4d1220375bb340309c12f67689448f4e7f for binaries and test log.

[gavinandresen]

My hidden agenda: I've got "reimplement NewThread to use boost::thread and get rid of the ugly fShutdown and vnThreadsRunning[] nonsense" near the top of my TODO list.

And last time I looked, boost::thread didn't expose a way to set thread priorities.

An #ifdef WINDOWS in the Qt startup code that sets the initial process' priority seems like a better approach, if running at lower priority makes sense.

[qubez]

Current master can hit the CPU pretty hard with it's multithreading, I caught up with 50 blocks and it pegged my quad core CPU at 100%, enough to stall my GPU miner. We would expect a similar hiccup about every 10 minutes. Reindexing or downloading blocks makes it hard to watch a video or other stuff without manually lowering priority or limiting processor affinity. Bitcoin could run with default lowered priority with little impact on it's operation.

Windows priority:
http://msdn.microsoft.com/en-us/library/ms686219%28v=vs.85%29.aspx

[sipa]

I think this needs to be below normal. In case of multithreaded sigchecking, it doesn't matter anyway (as it needs to wait on the sigcheck threads which are below normal and do the most cpu intensive work), and in single-threaded mode you don't want the import thread to kill your computer.

[Diapolo]

That is a simple thing to do, my intention was to give more priority to this thread to speed it up (I never did a benchmark though ^^), but what you say makes sense and I can update this pull. But Gavin said he doesn't want this fine grained priority settings for our threads, so I'm unsure if I should update or just close this pull.

[laanwj]

I don't know how windows thread scheduling exactly works, but are we sure
we're not just adjusting the priority that the thread gets within the
process? (the thread scheduling policy) If so, adjusting all these relative
priorities will do nothing for the system as a whole.

[Diapolo]

There is a priority class for the process (bitcoin-qt.exe), which is default and "normal", we are then able to tweak the resulting threads base priority by setting the threads priority level, which is what this pull does.

See http://msdn.microsoft.com/en-us/library/windows/desktop/ms685100%28v=vs.85%29.aspx for details.

[gavinandresen]

I finished my big thread cleanup: #2357

It replaces most calls to NewThread with calls to boost::thread_group::create_thread(), so is entirely incompatible with this pull.

Instead of trying to tweak individual thread priorities, maybe it makes more sense to pull the -par=negative patch, and have Bitcoin-Qt SoftSetArg("-par", "-1") so Bitcoin-Qt users get a free core by default (since the common use of bitcoind is on a dedicated server doing nothing but bitcoin, I think current behavior is just fine there).

[Diapolo]

I will take a look at your pull but agree, this one is obsolete and we should consider your next idea. IMHO it would be nice to still allow Bitcoin-Qt to use all cores, if a users explicitly wants that.

[laanwj]

Agreed Gavin, good idea to keep a free core for the ui+os to prevent
overall sluggishness.