gui: Improve thread naming #18790

hebasto commented at 10:08 PM on April 27, 2020: member

On master (eef90c14ed0f559e3f6e187341009270b84f45cb):

thread list from OS:
log excerpt:

2020-04-27T21:25:26Z [] GUI: initialize : Running initialization in thread
...
2020-04-27T21:26:04Z [] Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2020-04-27T21:26:04Z [] Using wallet /home/hebasto/.bitcoin/testnet3/wallets/test2
2020-04-27T21:26:04Z [] BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/test2/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/test2/db.log
2020-04-27T21:26:04Z [] init message: Loading wallet...
2020-04-27T21:26:04Z [] BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/test2/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/test2/db.log
2020-04-27T21:26:04Z [] [test2] Wallet File Version = 169900
2020-04-27T21:26:04Z [] [test2] Keys: 2001 plaintext, 0 encrypted, 2001 w/ metadata, 2001 total. Unknown wallet records: 0
2020-04-27T21:26:04Z [] [test2] Wallet completed loading in              26ms
2020-04-27T21:26:04Z [] GUI: TransactionTablePriv::refreshWallet
2020-04-27T21:26:04Z [] [test2] setKeyPool.size() = 2000
2020-04-27T21:26:04Z [] [test2] mapWallet.size() = 0
2020-04-27T21:26:04Z [] [test2] m_address_book.size() = 0

With this PR:

thread list from OS:
log excerpt:

2020-04-27T21:21:25Z [qt-init] GUI: initialize : Running initialization in thread
...
2020-04-27T21:23:08Z [qt-walletctrl] Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
2020-04-27T21:23:08Z [qt-walletctrl] Using wallet /home/hebasto/.bitcoin/testnet3/wallets/test2
2020-04-27T21:23:08Z [qt-walletctrl] BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/test2/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/test2/db.log
2020-04-27T21:23:08Z [qt-walletctrl] init message: Loading wallet...
2020-04-27T21:23:08Z [qt-walletctrl] BerkeleyEnvironment::Open: LogDir=/home/hebasto/.bitcoin/testnet3/wallets/test2/database ErrorFile=/home/hebasto/.bitcoin/testnet3/wallets/test2/db.log
2020-04-27T21:23:08Z [qt-walletctrl] [test2] Wallet File Version = 169900
2020-04-27T21:23:08Z [qt-walletctrl] [test2] Keys: 2001 plaintext, 0 encrypted, 2001 w/ metadata, 2001 total. Unknown wallet records: 0
2020-04-27T21:23:08Z [qt-walletctrl] [test2] Wallet completed loading in              37ms
2020-04-27T21:23:08Z [qt-walletctrl] init message: Rescanning...
2020-04-27T21:23:08Z [qt-walletctrl] [test2] Rescanning last 112924 blocks (from block 1609206)...
2020-04-27T21:23:08Z [qt-walletctrl] [test2] Rescan started from block 000000000000003761c81f7efbd8cebf217f39d353ec1ac59c624ac2dddfc2a8...
2020-04-27T21:23:22Z [qt-walletctrl] [test2] Rescan completed in           14157ms
2020-04-27T21:23:22Z [qt-walletctrl] GUI: TransactionTablePriv::refreshWallet
2020-04-27T21:23:22Z [qt-walletctrl] [test2] setKeyPool.size() = 2000
2020-04-27T21:23:22Z [qt-walletctrl] [test2] mapWallet.size() = 0
2020-04-27T21:23:22Z [qt-walletctrl] [test2] m_address_book.size() = 0

in src/txdb.cpp:253 in d8e3cf858c outdated

 248 | @@ -248,6 +249,9 @@ bool CBlockTreeDB::ReadFlag(const std::string &name, bool &fValue) {
 249 |  
 250 |  bool CBlockTreeDB::LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex)
 251 |  {
 252 | +    std::string thread_name = util::ThreadGetInternalName();
 253 | +    util::ThreadRename("leveldb");

MarcoFalke commented at 10:13 PM on April 27, 2020:

Why is this needed? Also, calling this leveldb is a layer-violation.

hebasto commented at 10:22 PM on April 27, 2020:

leveldb spawns a thread which inherits current thread name: "bitcoind" or "qt-init". Therefore, on master the OS reports about two "bitcoind" threads.

MarcoFalke commented at 12:39 AM on April 28, 2020:

Oh, I see. Maybe add a comment?

hebasto commented at 12:40 AM on April 28, 2020:

Mind suggesting a good wording?

laanwj commented at 8:22 AM on April 30, 2020:

To be honest, I think it's better to leave this out than do an ugly layer violation like this. At the least this would need to be documented very well, just having a scattered thread rename is confusing for anyone reading the code in the future. But I wonder if there's a better way to hook leveldb's thread creation and rename the threads.

hebasto commented at 8:29 AM on April 30, 2020:

Is it ok to just drop a commit with this change for better solution?

MarcoFalke commented at 11:17 AM on April 30, 2020:

It seemed weird anyway to to gui changes in the same pull request with leveldb changes

hebasto commented at 8:42 PM on April 30, 2020:

Updated.

DrahtBot commented at 11:01 PM on April 27, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

DrahtBot added the label GUI on Apr 27, 2020

DrahtBot added the label UTXO Db and Indexes on Apr 27, 2020

laanwj commented at 8:19 AM on April 30, 2020: member

Please keep to the convention of starting thread names with b-.

hebasto commented at 8:24 AM on April 30, 2020: member

Please keep to the convention of starting thread names with b-.

It is responsibility of the ThreadRename() function: https://github.com/bitcoin/bitcoin/blob/36c0abd8f61ba859d53b1e59014720282c97c143/src/util/threadnames.cpp#L57-L61

:)

UPDATE: On the screenshot in OP you can see all of the touched threads with b- prefix in the htop output.

laanwj commented at 8:56 AM on April 30, 2020: member

It is responsibility of the ThreadRename() function:

Thanks for clearing that up.

hebasto force-pushed on Apr 30, 2020

hebasto commented at 8:41 PM on April 30, 2020: member

Updated da42cd4ac1e1574a45578b04aab6e2a39653fc0e -> 64815f1f0da3431f2c2f880aeef4d57c3acd8aa5 (pr18790.01 -> pr18790.02, diff):

dropped "Name leveldb thread with its own name" commit

jb55 commented at 8:45 PM on April 30, 2020: member

ACK 64815f1f0da3431f2c2f880aeef4d57c3acd8aa5

MarcoFalke removed the label UTXO Db and Indexes on Apr 30, 2020

MarcoFalke renamed this:
~~Improve thread naming~~
gui: Improve thread naming
on Apr 30, 2020

brakmic commented at 9:53 AM on May 1, 2020: contributor

ACK 64815f1f0da3431f2c2f880aeef4d57c3acd8aa5

Built, run and tested on macOS Catalina 10.15.4

about_window

./src/qt/bitcoin-qt -logthreadnames -debug -printtoconsole -regtest                      (7s 63ms)  
2020-05-01T09:48:48Z Ignoring unknown configuration value regtest.natmap
2020-05-01T09:48:48Z [main] Bitcoin Core version pr18790.02 (release build)
2020-05-01T09:48:48Z [main] Qt 5.14.1 (dynamic), plugin=cocoa (dynamic)
2020-05-01T09:48:48Z [main] System: macOS 10.15, x86_64-little_endian-lp64
2020-05-01T09:48:48Z [main] Screen: Color LCD 1440x900, pixel ratio=1.0
2020-05-01T09:48:48Z [main] GUI: Populating font family aliases took 197 ms. Replace uses of missing font family ".AppleSystemUIFont,13,-1,5,50,0,0,0,0,0" with one that exists to avoid this cost. 
2020-05-01T09:48:48Z [main] Validating signatures for all blocks.
2020-05-01T09:48:48Z [main] Setting nMinimumChainWork=0000000000000000000000000000000000000000000000000000000000000000
2020-05-01T09:48:48Z [main] Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
2020-05-01T09:48:48Z [main] Using RdSeed as additional entropy source
2020-05-01T09:48:48Z [main] Using RdRand as an additional entropy source
2020-05-01T09:48:48Z [main] GUI: requestInitialize : Requesting initialize
2020-05-01T09:48:48Z [qt-init] GUI: initialize : Running initialization in thread
2020-05-01T09:48:48Z [qt-init] Default data directory /Users/brakmic/Library/Application Support/Bitcoin
2020-05-01T09:48:48Z [qt-init] Using data directory /Users/brakmic/Library/Application Support/Bitcoin/regtest
2020-05-01T09:48:48Z [qt-init] Config file: /Users/brakmic/Library/Application Support/Bitcoin/bitcoin.conf
2020-05-01T09:48:48Z [qt-init] Config file arg: regtest="0"
2020-05-01T09:48:48Z [qt-init] Config file arg: testnet="0"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] daemon="1"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] deprecatedrpc="signrawtransaction"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] listen="1"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] port="8444"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] prune="550"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] rest="1"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] rpcallowip="127.0.0.1"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] rpcbind=****
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] rpcpassword=****
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] rpcport="8445"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] rpcuser=****
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] server="1"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] shrinkdebugfile="1"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] zmqpubhashblock="tcp://127.0.0.1:58335"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] zmqpubhashtx="tcp://127.0.0.1:58334"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] zmqpubrawblock="tcp://127.0.0.1:58332"
2020-05-01T09:48:48Z [qt-init] Config file arg: [main] zmqpubrawtx="tcp://127.0.0.1:58333"

hebasto commented at 7:03 PM on May 1, 2020: member

@brakmic Thank you for reviewing. Mind particularly testing the changed threads: qt-clientmodl, qt-rpcconsole, and qt-walletctrl?

brakmic commented at 8:39 PM on May 1, 2020: contributor

@brakmic Thank you for reviewing. Mind particularly testing the changed threads: qt-clientmodl, qt-rpcconsole, and qt-walletctrl?

Yes, sure.

I was able to get qt-rpcconsole and qt-walletctrl but no matter what I do and whichever window I open, there is no qt-clientmodl in sight.

qt-rpcconsole

qt-walletctrl

hebasto commented at 8:44 PM on May 1, 2020: member

@brakmic

... but no matter what I do and whichever window I open, there is no qt-clientmodl in sight.

It's because there is no logging in the qt-clientmodl thread. But b-qt-clientmodl should be showed by the OS via tools similar to ps, top etc.

brakmic commented at 8:59 PM on May 1, 2020: contributor

@brakmic

... but no matter what I do and whichever window I open, there is no qt-clientmodl in sight.

It's because there is no logging in the qt-clientmodl thread. But b-qt-clientmodl should be showed by the OS via tools similar to ps, top etc.

Ok.

I took a sample from the running process and here's the b-qt-clientmodlthread.

qt-clientmodel

DrahtBot added the label Needs rebase on May 4, 2020

hebasto force-pushed on May 5, 2020

hebasto commented at 2:51 AM on May 5, 2020: member

Rebased 64815f1f0da3431f2c2f880aeef4d57c3acd8aa5 -> 6d8747dd13dc8d7c4868929041357ad4dda5aad8 (pr18790.02 -> pr18790.03) due to the conflict with #18699.

fanquake removed the label Needs rebase on May 5, 2020

DrahtBot added the label Needs rebase on May 19, 2020

hebasto force-pushed on May 19, 2020

hebasto commented at 4:01 PM on May 19, 2020: member

Rebased 6d8747dd13dc8d7c4868929041357ad4dda5aad8 -> f979110c851372b8702c3ae99e21f2e578640e4b (pr18790.03 -> pr18790.04) due to the conflict with #18152.

DrahtBot removed the label Needs rebase on May 19, 2020

promag commented at 11:54 AM on June 2, 2020: member

Concept ACK, build and dumped all thread list with lld, looks good.

From https://doc.qt.io/qt-5/qthread.html

To choose the name that your thread will be given (as identified by the command ps -L on Linux, for example), you can call setObjectName() before starting the thread. If you don't call setObjectName(), the name given to your thread will be the class name of the runtime type of your thread object (for example, "RenderThread" in the case of the Mandelbrot Example, as that is the name of the QThread subclass). Note that this is currently not available with release builds on Windows.

Is it because of the above windows issue that you don't use setObjectName? Not sure if that even maters to us.

hebasto commented at 11:57 AM on June 2, 2020: member

@promag

Is it because of the above windows issue that you don't use setObjectName? Not sure if that even maters to us.

Using setObjectName() does not work for our logging with thread names.

jonatack commented at 11:57 AM on June 5, 2020: member

ACK f979110

fanquake added the label Needs gitian build on Jun 8, 2020

DrahtBot commented at 6:21 AM on June 9, 2020: member

Gitian builds

File	commit 9573d2b55b45b24b9920cf5983d1681205ff41df<br>(master)	commit d7ac184b5963075bb77863288107e2bc7d099449<br>(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`e092833e8bc7a590...`	`ae361942d0011529...`
*-aarch64-linux-gnu.tar.gz	`9d4f23ed812ccc3c...`	`0173970597aa9fcd...`
*-arm-linux-gnueabihf-debug.tar.gz	`59794fcb8b141049...`	`6cb424f9746e755c...`
*-arm-linux-gnueabihf.tar.gz	`7dda786a40d23245...`	`729c3e9c93f50264...`
*-osx-unsigned.dmg	`f43b723fc0491aa5...`	`660ac8a6902be589...`
*-osx64.tar.gz	`4635b4bc5ecde0e2...`	`93f4f6c1c39414a0...`
*-riscv64-linux-gnu-debug.tar.gz	`bdd3128818116892...`	`9f0cd1313be1b8ee...`
*-riscv64-linux-gnu.tar.gz	`70e73b9bd66cc363...`	`df3c3d93fde6774f...`
*-win64-debug.zip	`6c3048443290eadc...`	`ccd7bff4d348726b...`
*-win64-setup-unsigned.exe	`2ceef94656b66c4b...`	`b404dcfa1029bbf3...`
*-win64.zip	`37c9e5183b6a6491...`	`4e4c2883500cce23...`
*-x86_64-linux-gnu-debug.tar.gz	`98c18ae66b7ff173...`	`3eca72e70ac170ed...`
*-x86_64-linux-gnu.tar.gz	`4d95d8abfe7665e1...`	`2c2dc98cbcd2c1bc...`
*.tar.gz	`b5211f42c22c6a7e...`	`07fcef7337a379de...`
bitcoin-core-linux-0.21-res.yml	`0d18708214e0fd79...`	`2f6bcb26066dc604...`
bitcoin-core-osx-0.21-res.yml	`4d41949eba545356...`	`2ae4cc85a53c6d9f...`
bitcoin-core-win-0.21-res.yml	`fa8a7240e220cc6e...`	`c69381443077fbbb...`
linux-build.log	`e9f33fa744efbeea...`	`35e3adfd71132f32...`
osx-build.log	`6589b020fa210d97...`	`a05934dbca3740ff...`
win-build.log	`58b4da2f4b194b38...`	`0ec5051ed72505fa...`
bitcoin-core-linux-0.21-res.yml.diff		`2194cbd0d06cc034...`
bitcoin-core-osx-0.21-res.yml.diff		`104571e0f443b20f...`
bitcoin-core-win-0.21-res.yml.diff		`8e6fafb15a072c79...`
linux-build.log.diff		`a2f14aa723150c51...`
osx-build.log.diff		`1566cb8e53954cae...`
win-build.log.diff		`60da6f8ac6b4bc5b...`

DrahtBot removed the label Needs gitian build on Jun 9, 2020

Sjors commented at 2:32 PM on June 19, 2020: member

Concept ACK

DrahtBot added the label Needs rebase on Aug 13, 2020

qt: Name RPCConsole executor QThread 27dcc37d42

qt: Name WalletController worker QThread 2c7f5d8c2e

qt: Name ClientModel timer QThread ad5f614bf3

qt: Rename qt-init thread before logging start ead771bf6f

hebasto force-pushed on Aug 13, 2020

hebasto commented at 1:42 PM on August 13, 2020: member

Rebased f979110c851372b8702c3ae99e21f2e578640e4b -> ead771bf6fc7a4b96a03d4938796c88657c69ba6 (pr18790.04 -> pr18790.05) due to the conflict with #19011.

DrahtBot removed the label Needs rebase on Aug 13, 2020

fanquake commented at 8:41 AM on September 15, 2020: member

Planning on reviewing (shortly) and merging this PR in this repo. @promag @jb55 @jonatack @Sjors want to take another look here?

Sjors commented at 11:20 AM on September 17, 2020: member

tACK ead771bf6fc7a4b96a03d4938796c88657c69ba6

fanquake merged this on Sep 19, 2020

fanquake closed this on Sep 19, 2020

hebasto deleted the branch on Sep 19, 2020

sidhujag referenced this in commit af78e21b6c on Sep 20, 2020

Fabcien referenced this in commit 398c7720aa on Oct 11, 2021

DrahtBot locked this on Feb 15, 2022