Rework the clients thread priority handling #2199

Diapolo commented at 5:23 PM on January 22, 2013: none

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 commented at 7:27 PM on January 22, 2013: contributor

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 commented at 7:46 PM on January 22, 2013: member

@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 commented at 8:20 PM on January 22, 2013: none

@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 commented at 10:08 PM on January 22, 2013: member

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.

Rework the clients thread priority handling

- 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

f2cb7f4d12

in src/util.cpp:None in 9b8a86031f outdated

1385 | +#ifdef WIN32
1386 | +        bool bResult = SetThreadPriority(newThread.native_handle(), nPriority);
1387 | +        if (!bResult)
1388 | +            printf("NewThread() : Error setting thread priority\n");
1389 | +#else
1390 | +        // It's unclear if it's even possible to change thread priorities on Linux,

laanwj commented at 10:10 PM on January 22, 2013:

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 commented at 6:32 AM on January 23, 2013:

An update is on the way ;).

Diapolo commented at 6:39 AM on January 23, 2013: none

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 commented at 2:02 AM on January 24, 2013: none

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

gavinandresen commented at 2:18 AM on January 24, 2013: contributor

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 commented at 9:48 PM on February 14, 2013: none

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

in src/init.cpp:None in f2cb7f4d12

 940 | @@ -941,7 +941,7 @@ bool AppInit2()
 941 |          BOOST_FOREACH(string strFile, mapMultiArgs["-loadblock"])
 942 |              pimport->vFiles.push_back(strFile);
 943 |      }
 944 | -    NewThread(ThreadImport, pimport);
 945 | +    NewThread(ThreadImport, pimport, THREAD_PRIORITY_ABOVE_NORMAL);

sipa commented at 10:01 PM on February 14, 2013:

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 commented at 7:12 AM on February 15, 2013:

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 commented at 8:33 AM on February 15, 2013:

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 commented at 10:18 AM on February 15, 2013:

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 commented at 11:31 PM on March 11, 2013: contributor

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 commented at 7:11 AM on March 12, 2013: none

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.

Diapolo closed this on Mar 12, 2013

laanwj commented at 7:42 AM on March 12, 2013: member

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

DrahtBot locked this on Sep 8, 2021