refactor: Replace thread_local use with a mutex-protected map #18851

laanwj · 2020-05-02T10:46:20Z

This replaces the only use of thread_local with a mutex-protected map.

This leaks per thread, but because bitcoin's threads are static and permanent, this is not a problem in practice. Add a note to the thread rename function to only call it for permanent threads, just in case.

Also removes associated build system functionality.

Closes #18678.

This replaces the only use of `thread_local` with a mutex-protected map. This leaks per thread, but because bitcoin's threads are static and permanent, this is not a problem in practice. Add a note to the thread rename function to only call it for permanent threads, just in case. Also removes associated build system functionality. Closes bitcoin#18678.

meshcollider · 2020-05-02T11:20:46Z

Concept / code review ACK c793389 modulo someone testing that it does fix the performance issue linked

maflcko · 2020-05-02T11:47:49Z

Concept ACK if this fixes the problem. Will test when my new hardware arrives

laanwj · 2020-05-02T11:57:24Z

If this doesn't solve the problem, then the problem isn't thread_local but looking up thread names in the first place, and we should remove thread name logging completely (as well as thread_local).

A workaround then that would avoid the need for this bookkeeping at all would be:

Log thread names once with their associated ID after creation on thread name setting
Log thread IDs instead of names in log messages

DrahtBot · 2020-05-02T12:27:59Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

The Zero Allocations project #18849 (The Zero Allocations project by jb55)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto · 2020-05-02T19:18:55Z

Concept ACK. Keep testing.

hebasto · 2020-05-02T20:40:40Z

Testing on ODROID-HC1:

$ uname -srm
Linux 4.14.173-173 armv7l
$ ./autogen.sh && ./configure --disable-wallet --disable-tests --disable-bench --disable-zmq --disable-man --disable-util-tx --disable-util-wallet --without-miniupnpc --without-libs --without-gui CC=clang-9 CXX=clang++-9 && make clean && time taskset --cpu-list 4-7 make --jobs=4

UPDATE: sorry, wrong branch compiled

hebasto · 2020-05-03T00:42:40Z

Here are results of three consequent tests on ODROID-HC1:

$ uname -srm
Linux 4.14.173-173 armv7l
$ date; time bitcoin-cli -rpcclienttimeout=10000 gettxoutsetinfoSat May  2 23:18:12 UTC 2020
{
  "height": 628635,
  "bestblock": "0000000000000000000210066becbfb2ebfdd957d3bdaec7190f46d87ce59b7a",
  "transactions": 39898110,
  "txouts": 66828188,
  "bogosize": 5019712922,
  "hash_serialized_2": "78c80aed68a14274a1dbadd9213ff6da55618d29c2b077c8f502ce5f900fcd03",
  "disk_size": 4027535280,
  "total_amount": 18357767.32081895
}

real	6m32.011s
user	0m0.030s
sys	0m0.020s
$ date; time bitcoin-cli -rpcclienttimeout=10000 gettxoutsetinfoSat May  2 23:31:50 UTC 2020
{
  "height": 628635,
  "bestblock": "0000000000000000000210066becbfb2ebfdd957d3bdaec7190f46d87ce59b7a",
  "transactions": 39898110,
  "txouts": 66828188,
  "bogosize": 5019712922,
  "hash_serialized_2": "78c80aed68a14274a1dbadd9213ff6da55618d29c2b077c8f502ce5f900fcd03",
  "disk_size": 4035885849,
  "total_amount": 18357767.32081895
}

real	9m48.031s
user	0m0.004s
sys	0m0.040s
$ date; time bitcoin-cli -rpcclienttimeout=10000 gettxoutsetinfoSat May  2 23:44:47 UTC 2020
{
  "height": 628636,
  "bestblock": "0000000000000000000e91c7a666359c09a405e3a5294181100a6aeb7d9d6427",
  "transactions": 39898996,
  "txouts": 66829911,
  "bogosize": 5019841104,
  "hash_serialized_2": "065cb1955ffc90c37ef293d8bee8e2ca9af9f9d87b0a50f8d58b4b874b642a82",
  "disk_size": 4030980759,
  "total_amount": 18357779.82081895
}

real	53m16.860s
user	0m0.008s
sys	0m0.035s

The related log:

2020-05-02T21:11:03Z [init] Bitcoin Core version v0.20.99.0-c7933899e (release build)
2020-05-02T21:11:03Z [init] InitParameterInteraction: parameter interaction: -externalip set -> setting -discover=0
2020-05-02T21:11:03Z [init] Assuming ancestors of block 0000000000000000000f2adce67e49b0b6bdeb9de8b7c3d7e93b21e7fc1e819d have valid signatures.
2020-05-02T21:11:03Z [init] Setting nMinimumChainWork=00000000000000000000000000000000000000000e1ab5ec9348e9f4b8eb8154
2020-05-02T21:11:03Z [init] Using the 'standard' SHA256 implementation
2020-05-02T21:11:03Z [init] Default data directory /home/core/.bitcoin
2020-05-02T21:11:03Z [init] Using data directory /var/lib/bitcoind
2020-05-02T21:11:03Z [init] Config file: /etc/bitcoin/bitcoin.conf
2020-05-02T21:11:03Z [init] Config file arg: debug="rpc"
2020-05-02T21:11:03Z [init] Config file arg: externalip="95.164.65.194"
2020-05-02T21:11:03Z [init] Config file arg: listenonion="0"
2020-05-02T21:11:03Z [init] Config file arg: logthreadnames="1"
2020-05-02T21:11:03Z [init] Config file arg: par="4"
2020-05-02T21:11:03Z [init] Config file arg: rpcthreads="2"
2020-05-02T21:11:03Z [init] Config file arg: rpcworkqueue="1"
...
2020-05-02T23:18:12Z [httpworker.0] ThreadRPCServer method=gettxoutsetinfo user=__cookie__
2020-05-02T23:18:12Z [httpworker.0] FlushStateToDisk: write coins cache to disk (337567 coins, 39627kB) started
2020-05-02T23:18:14Z [httpworker.0] FlushStateToDisk: write coins cache to disk (337567 coins, 39627kB) completed (2.05s)
2020-05-02T23:31:50Z [httpworker.1] ThreadRPCServer method=gettxoutsetinfo user=__cookie__
2020-05-02T23:31:51Z [httpworker.1] FlushStateToDisk: write coins cache to disk (3522 coins, 2177kB) started
2020-05-02T23:31:51Z [httpworker.1] FlushStateToDisk: write coins cache to disk (3522 coins, 2177kB) completed (0.02s)
2020-05-02T23:40:36Z [msghand] UpdateTip: new best=0000000000000000000e91c7a666359c09a405e3a5294181100a6aeb7d9d6427 height=628636 version=0x27ffe000 log2_work=91.914443 tx=526304521 date='2020-05-02T23:40:05Z' progress=1.000000 cache=3.2MiB(13600txo) warning='67 of last 100 blocks have unexpected version'
2020-05-02T23:44:47Z [httpworker.0] ThreadRPCServer method=gettxoutsetinfo user=__cookie__
2020-05-02T23:44:48Z [httpworker.0] FlushStateToDisk: write coins cache to disk (14579 coins, 3418kB) started
2020-05-02T23:44:48Z [httpworker.0] FlushStateToDisk: write coins cache to disk (14579 coins, 3418kB) completed (0.09s)
2020-05-02T23:59:13Z [msghand] UpdateTip: new best=000000000000000000034cd30624babe78f29f0944b2e73d784198d4fd1af594 height=628637 version=0x20000000 log2_work=91.914464 tx=526307327 date='2020-05-02T23:54:25Z' progress=0.999998 cache=3.1MiB(12818txo) warning='66 of last 100 blocks have unexpected version'
2020-05-02T23:59:30Z [msghand] New outbound peer connected: version: 70015, blocks=628638, peer=1217 (full-relay)
2020-05-03T00:08:15Z [msghand] UpdateTip: new best=0000000000000000000a9c8ea50980bece5f9168e962c868e2f9d9761d5ddb0c height=628638 version=0x20800000 log2_work=91.914485 tx=526308404 date='2020-05-02T23:54:28Z' progress=0.999995 cache=4.0MiB(21407txo) warning='67 of last 100 blocks have unexpected version'
2020-05-03T00:08:15Z [net] socket sending timeout: 1382s
2020-05-03T00:14:30Z [msghand] UpdateTip: new best=00000000000000000006ceea137d6caebd533d6fd95f8c1da6be15fe01c6d828 height=628639 version=0x20c00000 log2_work=91.914506 tx=526310651 date='2020-05-03T00:08:22Z' progress=0.999998 cache=5.0MiB(30426txo) warning='67 of last 100 blocks have unexpected version'
2020-05-03T00:32:24Z [msghand] UpdateTip: new best=0000000000000000000d0ce69147e7c831a0ea999c5837667a2928960d538cce height=628640 version=0x37ffe000 log2_work=91.914527 tx=526313977 date='2020-05-03T00:31:10Z' progress=1.000000 cache=6.1MiB(41548txo) warning='67 of last 100 blocks have unexpected version'

hebasto · 2020-05-03T09:05:13Z

It seems thread_local is not the root of the performance degradation.
Please see #18538 (comment)

DrahtBot · 2020-05-04T05:38:42Z

Gitian builds

File	commit `68ef952` (master)	commit 4c270d2f47d244ef37c4aa1377c0d56b3b922bef (master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`f563236ed2fe55e7...`	`933e19338f6026a2...`
*-aarch64-linux-gnu.tar.gz	`0137ede5b5a3b69d...`	`04edf669faf37a18...`
*-arm-linux-gnueabihf-debug.tar.gz	`6d981a7c8574b29e...`	`7d83f08b6e80ffcd...`
*-arm-linux-gnueabihf.tar.gz	`67333c529c76f1b1...`	`979373626368c265...`
*-osx-unsigned.dmg	`ac9fe9134e61be05...`	`b84109845e585992...`
*-osx64.tar.gz	`b244355f3f895985...`	`7c0b07f30f22c3b2...`
*-riscv64-linux-gnu-debug.tar.gz	`e41c9f54fbf8658e...`	`72209385bc8b457a...`
*-riscv64-linux-gnu.tar.gz	`59c0518eadf4a01e...`	`030748544a4c7572...`
*-win64-debug.zip	`3fc1adaa18cf42a3...`	`f38b9c97640b1336...`
*-win64-setup-unsigned.exe	`ab3a4cdcd55c2557...`	`48762f310eb61f8f...`
*-win64.zip	`86b1e7c9fbce4e1b...`	`bec39b501c6015e1...`
*-x86_64-linux-gnu-debug.tar.gz	`af837f75521add5d...`	`81e210871dd6bb84...`
*-x86_64-linux-gnu.tar.gz	`9a0c056928b7b1b7...`	`4e6a632a671596c9...`
*.tar.gz	`b7e353469956e000...`	`95d1e48eefb39003...`
bitcoin-core-linux-0.21-res.yml	`99533cb54b367d2b...`	`a7487bf63e97549a...`
bitcoin-core-osx-0.21-res.yml	`3d3333a8ce2e234c...`	`7a10a1e2cbf860b4...`
bitcoin-core-win-0.21-res.yml	`2d2bcab124ead9ec...`	`5852959f44562090...`
linux-build.log	`badd1a4ca8d67232...`	`a006fdb1bb797e8f...`
osx-build.log	`0c305199e0e3ebb6...`	`376adbf1fc9d0a47...`
win-build.log	`ff86639c1ee0f14a...`	`6f1f9a6ce56d0410...`
bitcoin-core-linux-0.21-res.yml.diff		`e25c9488f31761c5...`
bitcoin-core-osx-0.21-res.yml.diff		`d1fa1259824d80ea...`
bitcoin-core-win-0.21-res.yml.diff		`6d751cb9f972f0d2...`
linux-build.log.diff		`d9417d859fc67f71...`
osx-build.log.diff		`c1307497c3bfedc6...`
win-build.log.diff		`f063a09a95217973...`

laanwj · 2020-05-04T13:18:18Z

Okay. So is it still worth keeping this open? It would make thread-name logging work in the gitian-built binaries which lack thread_local. And cleans up the build system a bit.

hebasto · 2020-05-04T13:20:21Z

It would make thread-name logging work in the gitian-built binaries which lack thread_local. And cleans up the build system a bit.

Correct. So keep reviewing.

maflcko · 2020-05-04T13:32:25Z

I am currently syncing a node to debug this issue.

Also, glibc can probably be bumped by one minor version in the next release, which is going to switch to C++17. Then, potentially the same build cleanups can be done?

laanwj added the Refactoring label May 2, 2020

fanquake added the Utils/log/libs label May 2, 2020

laanwj mentioned this pull request May 2, 2020

Rethink use of thread_local #18678

Closed

laanwj force-pushed the 2020_05_replace_threadlocal branch from 78031df to c793389 Compare May 2, 2020 10:52

DrahtBot mentioned this pull request May 2, 2020

The Zero Allocations project #18849

Closed

4 tasks

maflcko added Needs gitian build DrahtBot Guix build requested labels May 2, 2020

hebasto mentioned this pull request May 3, 2020

gitian vs self-compiled on ARM 32-bit gettxoutsetinfo #18538

Closed

DrahtBot removed the Needs gitian build label May 4, 2020

laanwj closed this May 4, 2020

maflcko removed the DrahtBot Guix build requested label May 6, 2020

maflcko mentioned this pull request May 18, 2020

Prevent UB in DeleteLock() function #18881

Merged

laanwj mentioned this pull request Jan 25, 2021

gitian-linux: Build binaries for 64-bit POWER (continued) #20963

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

refactor: Replace thread_local use with a mutex-protected map #18851

refactor: Replace thread_local use with a mutex-protected map #18851

Uh oh!

Conversation

laanwj commented May 2, 2020

Uh oh!

meshcollider commented May 2, 2020

Uh oh!

maflcko commented May 2, 2020

Uh oh!

laanwj commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 2, 2020

Conflicts

Uh oh!

hebasto commented May 2, 2020

Uh oh!

hebasto commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 3, 2020

Uh oh!

hebasto commented May 3, 2020

Uh oh!

DrahtBot commented May 4, 2020

Gitian builds

Uh oh!

laanwj commented May 4, 2020

Uh oh!

hebasto commented May 4, 2020

Uh oh!

maflcko commented May 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

laanwj commented May 2, 2020 •

edited

Loading

hebasto commented May 2, 2020 •

edited

Loading