Thread names in logs and deadlock debug tools

jamesob commented at 9:14 pm on April 18, 2019: member

I’m resurrecting this one (from #13168) because I need it to make progress on #15735.

It’s now off by default and can be turned on with -logthreadnames=1.

Ran some benchmarks (IBD from local peer from 500_000 -> 504_000) and it’s within spitting distance either on or off:

threadnames off (default)

2018-05-threadnames.3 vs. master (absolute)

name	iterations	2018-05-threadnames.3	master
ibd.local.500000.504000.dbcache=2048	3	376.1584 (± 9.2944)	392.3414 (± 13.4238)
ibd.local.500000.504000.dbcache=2048.mem-usage	3	2236117.3333 (± 1845.9623)	2238690.6667 (± 2669.3487)

2018-05-threadnames.3 vs. master (relative)

name	iterations	2018-05-threadnames.3	master
ibd.local.500000.504000.dbcache=2048	3	1	1.043
ibd.local.500000.504000.dbcache=2048.mem-usage	3	1	1.001

threadnames on

2018-05-threadnames-take-2 vs. master (absolute)

name	iterations	2018-05-threadnames-take-2	master
ibd.local.500000.504000.dbcache=2048	3	367.6861 (± 0.3941)	364.1667 (± 0.9776)
ibd.local.500000.504000.dbcache=2048.mem-usage	3	2238461.3333 (± 3697.8730)	2237014.6667 (± 3307.6966)

2018-05-threadnames-take-2 vs. master (relative)

name	iterations	2018-05-threadnames-take-2	master
ibd.local.500000.504000.dbcache=2048	3	1.010	1.00
ibd.local.500000.504000.dbcache=2048.mem-usage	3	1.001	1.00

jamesob force-pushed on Apr 18, 2019

DrahtBot commented at 9:25 pm on April 18, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15606 ([experimental] UTXO snapshots by jamesob)
#15493 (rfc: Add -printconfig arg to bitcoind by promag)
#15382 ([util] add runCommandParseJSON by Sjors)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)

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.

fanquake added the label Utils/log/libs on Apr 18, 2019

practicalswift commented at 11:33 am on April 23, 2019: contributor

Concept ACK

laanwj added this to the "Blockers" column in a project

in src/logging.cpp:210 in a0ca62b189 outdated

211 {
212-    std::string strTimestamped = LogTimestampStr(str);
213+    std::string strPrefixed = str;
214+
215+    if (m_log_threadnames && m_started_new_line)
216+        strPrefixed.insert(0, "[" + std::move(thread_util::GetInternalName()) + "] ");

practicalswift commented at 9:11 pm on April 25, 2019:

I think this is a pessimizing move which prevents copy elision.

jamesob commented at 1:59 pm on April 26, 2019:

Fixed, thanks.

in src/sync.cpp:44 in a0ca62b189 outdated

46-        sourceLine = nLine;
47-        fTry = fTryIn;
48-    }
49+    CLockLocation(
50+        const char* pszName, const char* pszFile, int nLine, bool fTryIn, std::string thread_name_) :
51+            mutexName(pszName), sourceFile(pszFile), sourceLine(nLine), fTry(fTryIn),

practicalswift commented at 9:16 pm on April 25, 2019:

Reorder to match the order the compiler will use (which is the order of declaration): so please put fTry first :-)

jamesob commented at 1:59 pm on April 26, 2019:

Fixed, thanks.

jamesob force-pushed on Apr 26, 2019

in configure.ac:830 in c1d159999c outdated

826@@ -827,8 +827,23 @@ AC_LINK_IFELSE([AC_LANG_SOURCE([
827   }
828   ])],
829   [
830-    AC_DEFINE(HAVE_THREAD_LOCAL,1,[Define if thread_local is supported.])
831-    AC_MSG_RESULT(yes)
832+   case $host in

ryanofsky commented at 4:14 pm on April 26, 2019:

In commit “disable thread_local on unreliable platforms” (c1d159999c6fc2b81fb8d0caf7d68a744d127fb8)

Commit message is misleading. It sounds like this disables the thread_local keyword when actually it disables the HAVE_THREAD_LOCAL macro. Would suggest: “disable HAVE_THREAD_LOCAL on unreliable platforms” and mentioning that this doesn’t affect anything except the DEBUG_LOCKCONTENTION implementation.

in src/Makefile.am:197 in 91cc480ddd outdated

193@@ -194,6 +194,7 @@ BITCOIN_CORE_H = \
194   support/events.h \
195   support/lockedpool.h \
196   sync.h \
197+  threadutil.h \

ryanofsky commented at 4:29 pm on April 26, 2019:

In commit “threads: introduce threadutil, refactor thread naming” (91cc480ddde33e92d87795d72ff28658bcc88877)

Now that we have a util directory, I’d suggest calling this util/threadnames.h or util/threads.h.

I’d also suggest going with util::ThreadRename instead of thread_util::Rename. Even though we’re only doing it a few places, and it’s less important in smaller projects like ours, I think its nice when c++ namespaces just mirror directory paths:

https://github.com/bitcoin/bitcoin/blob/5046d4e911aa5ef4d546e8a6d66ca1810c89592a/src/util/system.h#L369 https://github.com/bitcoin/bitcoin/blob/5046d4e911aa5ef4d546e8a6d66ca1810c89592a/src/interfaces/chain.h#L28

in src/threadutil.h:12 in 91cc480ddd outdated

 7+
 8+#include <string>
 9+
10+namespace thread_util
11+{
12+    //! Rename a thread both in terms of an internal (in-memory) name as well

ryanofsky commented at 4:34 pm on April 26, 2019:

In commit “threads: introduce threadutil, refactor thread naming” (91cc480ddde33e92d87795d72ff28658bcc88877)

Suggest formatting with clang-format. Namespace contents shouldn’t be indented here.

in src/threadutil.h:18 in 91cc480ddd outdated

13+    //! as its system thread name.
14+    void Rename(const std::string&);
15+
16+    //! Get the thread's internal (in-memory) name; used e.g. for identification in
17+    //! logging.
18+    std::string GetInternalName();

ryanofsky commented at 4:45 pm on April 26, 2019:

In commit “threads: introduce threadutil, refactor thread naming” (91cc480ddde33e92d87795d72ff28658bcc88877)

Not sure if you still have any efficiency concerns at this point, but just in case you do, it would be possible to avoid a copy and allocation here by changing the return type to const std::string&

in src/threadutil.cpp:22 in 91cc480ddd outdated

17+
18+//! Set the thread's name at the process level. Does not affect the
19+//! internal name.
20+static void SetThreadName(const char* name)
21+{
22+#if defined(PR_SET_NAME)

ryanofsky commented at 4:48 pm on April 26, 2019:

In commit “threads: introduce threadutil, refactor thread naming” (91cc480ddde33e92d87795d72ff28658bcc88877)

Note: Maybe obvious, but just in case it helps other reviewers, this code is unchanged (easy to verify with git log -p -n1 --color-moved=dimmed_zebra)

in src/test/threadutil_tests.cpp:37 in 553a4d660b outdated

32+    std::mutex lock;
33+
34+    auto RenameThisThread = [&](int i) {
35+        thread_util::Rename(TEST_THREAD_NAME_BASE + std::to_string(i));
36+        std::lock_guard<std::mutex> guard(lock);
37+        names.insert(thread_util::GetInternalName());

ryanofsky commented at 4:53 pm on April 26, 2019:

In commit “tests: add threadutil tests” (553a4d660b3cd4b9ed1ea66b780bf001081eaa64)

Nice test!

in src/logging.cpp:8 in 4fba3ead98 outdated

4@@ -5,6 +5,9 @@
5 
6 #include <logging.h>
7 #include <util/time.h>
8+#include <threadutil.h>

ryanofsky commented at 4:55 pm on April 26, 2019:

In commit “threads: prefix log messages with thread names” (4fba3ead9811ebe47161e9e7f535a7cc88403cf5)

Could sort alphabetically (thread before util)

in src/sync.cpp:61 in 503c1ff6ee outdated

64 
65 private:
66     bool fTry;
67     std::string mutexName;
68     std::string sourceFile;
69+    std::string thread_name;

ryanofsky commented at 5:06 pm on April 26, 2019:

In commit “threads: add thread names to deadlock debugging message” (503c1ff6eeeb6e222ff446033745650ba59ab659)

In case you do take my earlier suggestion and change GetInternalName to return a const string reference instead of a temporary string, you could change this type to a const string reference and avoid any string copy initializing this struct field (it would point directly to the g_thread_name instance).

jamesob commented at 7:02 pm on April 26, 2019:

In making this change I hit some compiler barf that has me out of my depths: https://gist.github.com/jamesob/5a19c75b34a49a4972b4105e0287d79d

ryanofsky commented at 7:41 pm on April 26, 2019:

Oh, the relevant part of that error message is:

0sync.cpp:133:33: note: in instantiation of member function 'std::vector<std::pair<void *, CLockLocation>, std::allocator<std::pair<void *, CLockLocation> > >::operator=' requested here
1        lockdata.lockorders[p1] = g_lockstack;

It’s complaining that it can’t copy the g_lockstack variable, which makes sense since adding a reference member to CLockLocation makes it not copyable by default. If inclined, you could work around it by manually defining a CLockLocation copy constructor:

0CLockLocation(const CLockLocation& other) : fTry(other.fTry), mutexName(other.mutexName), etc, ... {}

ryanofsky commented at 8:03 pm on April 26, 2019:

I think I was wrong. Even with a reference member CLockLocation should still be copyable, just not assignable, so you may be able to fix this by emplacing into lockorders instead of assigning into it.

jamesob commented at 8:04 pm on April 26, 2019:

Fixed by changing the above assignment to emplace() per your advice. Thanks!

in src/logging.cpp:180 in 4fba3ead98 outdated

176@@ -174,7 +177,7 @@ std::vector<CLogCategoryActive> ListActiveLogCategories()
177     return ret;
178 }
179 
180-std::string BCLog::Logger::LogTimestampStr(const std::string &str)
181+std::string BCLog::Logger::LogTimestampStr(const std::string str)

ryanofsky commented at 5:13 pm on April 26, 2019:

In commit “threads: prefix log messages with thread names” (4fba3ead9811ebe47161e9e7f535a7cc88403cf5)

Not sure if you dropped the & intentionally here. It would make sense to drop it, so the string could be moved from instead of copied. But you’d also want to drop the const and add std::move below, and also change the Logger::LogPrintStr function similarly (function calling this function).

Without these things, I don’t think there’d be a reason to drop &.

in src/logging.cpp:214 in 4fba3ead98 outdated

215+    if (m_log_threadnames && m_started_new_line)
216+        strPrefixed.insert(0, "[" + thread_util::GetInternalName() + "] ");
217+
218+    strPrefixed = LogTimestampStr(strPrefixed);
219+
220+    m_started_new_line = (!str.empty() && str[str.size()-1] == '\n');

ryanofsky commented at 5:17 pm on April 26, 2019:

In commit “threads: prefix log messages with thread names” (4fba3ead9811ebe47161e9e7f535a7cc88403cf5)

Some unneeded parentheses here

in src/logging.cpp:207 in 4fba3ead98 outdated

208 }
209 
210 void BCLog::Logger::LogPrintStr(const std::string &str)
211 {
212-    std::string strTimestamped = LogTimestampStr(str);
213+    std::string strPrefixed = str;

ryanofsky commented at 5:20 pm on April 26, 2019:

In commit “threads: prefix log messages with thread names” (4fba3ead9811ebe47161e9e7f535a7cc88403cf5)

Since this is renaming, could go to with current lowercase convention.

ryanofsky approved

ryanofsky commented at 5:30 pm on April 26, 2019: member

utACK 503c1ff6eeeb6e222ff446033745650ba59ab659. Looks like some windows tests are failing, but nothing to do with this PR. I left a few suggestions for changes, but please ignore any that do not sound like super great and wonderful ideas.

disable HAVE_THREAD_LOCAL on unreliable platforms

Note that this doesn't affect anything unless
DEBUG_LOCKCONTENTION is defined.

See discussions here:

- https://github.com/bitcoin/bitcoin/pull/11722#pullrequestreview-79322658
- https://github.com/bitcoin/bitcoin/pull/13168#issuecomment-387181155

188ca75e5f

jamesob force-pushed on Apr 26, 2019

jamesob commented at 7:05 pm on April 26, 2019: member

@ryanofsky thanks very much for the review. Per usual, your suggested changes are indelibly righteous. I’ve made all of them save for the last I commented on.

jamesob commented at 8:00 pm on April 26, 2019: member

Reran benchmarking; no performance difference.

absolute

name	iterations	master	threadnames2.18
build.make.23.clang	1	135.8741 (± 0.0000)	133.9639 (± 0.0000)
build.make.23.clang.mem-usage	1	648692.0000 (± 0.0000)	646632.0000 (± 0.0000)
ibd.local.500000.504000.dbcache=2048	3	357.4552 (± 1.2016)	357.3936 (± 1.1932)
ibd.local.500000.504000.dbcache=2048.mem-usage	3	2240597.3333 (± 3291.6968)	2237538.6667 (± 4462.0143)

relative

name	iterations	master	threadnames2.18
build.make.23.clang	1	1.014	1
build.make.23.clang.mem-usage	1	1.003	1
ibd.local.500000.504000.dbcache=2048	3	1.000	1
ibd.local.500000.504000.dbcache=2048.mem-usage	3	1.001	1

jamesob force-pushed on Apr 26, 2019

in src/util/threadnames.cpp:48 in 1cf8779ee2 outdated

43+static void SetInternalName(const std::string& name) { g_thread_name = name; }
44+
45+// Without thread_local available, don't handle internal name at all.
46+#else
47+
48+const std::string& util::ThreadGetInternalName() { return ""; }

ryanofsky commented at 8:16 pm on April 26, 2019:

In commit “threads: introduce threadutil, refactor thread naming” (1cf8779ee2887e3c8ea0265fe8bbb9ecbd502172)

Sorry, this is from a change I suggested, but it isn’t really safe (and causes a warning):

0util/threadnames.cpp:48:59: warning: returning reference to local temporary object [-Wreturn-stack-address]
1const std::string& util::ThreadGetInternalName() { return ""; }

Should do something like:

0static const std::string empty_string;
1const std::string& util::ThreadGetInternalName() { return empty_string; }

in src/util/threadnames.h:6 in 1cf8779ee2 outdated

0@@ -0,0 +1,21 @@
1+// Copyright (c) 2018 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_THREADUTIL_H
6+#define BITCOIN_THREADUTIL_H

ryanofsky commented at 8:19 pm on April 26, 2019:

In commit “threads: introduce threadutil, refactor thread naming” (1cf8779ee2887e3c8ea0265fe8bbb9ecbd502172)

Should use BITCOIN_UTIL_THREADNAMES_H. Also commit message text reference previous names and could be updated.

ryanofsky approved

ryanofsky commented at 8:20 pm on April 26, 2019: member

utACK 2e53d900ba40623b203bf21fc572a0fe56669fc7. Just various suggested changes since last review.

jamesob force-pushed on Apr 26, 2019

jamesob commented at 8:26 pm on April 26, 2019: member

@ryanofsky Oops! Thanks, pushed.

ryanofsky approved

ryanofsky commented at 9:08 pm on April 26, 2019: member

utACK eed4a5c7f81b53e47df3c4505f2b273a25fc9980. Reference to temporary bug’s fixed now. There is still the lint problem causing travis to fail, though (https://travis-ci.org/bitcoin/bitcoin/jobs/525122780#L356).

jamesob force-pushed on Apr 26, 2019

in src/test/util_threadnames_tests.cpp:21 in 043a387639 outdated

16+
17+#include <boost/test/unit_test.hpp>
18+
19+BOOST_FIXTURE_TEST_SUITE(util_threadnames_tests, BasicTestingSetup)
20+
21+std::string TEST_THREAD_NAME_BASE = "test_thread.";

MarcoFalke commented at 10:03 pm on April 26, 2019:

in commit 195ba388eb tests: add threadutil tests

style-nit: Should be const, since it is upper cased?

jamesob commented at 10:47 pm on April 26, 2019:

Done, thanks.

in doc/release-notes-pr13168.md:6 in 043a387639 outdated

0@@ -0,0 +1,6 @@
1+Thread names in logs
2+--------------------
3+
4+On platforms supporting `thread_local`, log lines are now prefixed by default
5+with the name of the thread that caused the log. To disable this behavior,
6+specify `-logthreadnames=0`.

MarcoFalke commented at 10:10 pm on April 26, 2019:

Why does the OP say it is off by default, but the code and release notes otherwise?

0 static const bool DEFAULT_LOGTHREADNAMES = true;

jamesob commented at 10:40 pm on April 26, 2019:

Wowow, something must’ve gotten screwed up in a rebase. Fixing - thanks for the catch.

in src/logging.cpp:210 in 043a387639 outdated

211 {
212-    std::string strTimestamped = LogTimestampStr(str);
213+    std::string str_prefixed = str;
214+
215+    if (m_log_threadnames && m_started_new_line)
216+        str_prefixed.insert(0, "[" + util::ThreadGetInternalName() + "] ");

MarcoFalke commented at 10:20 pm on April 26, 2019:

style-nit: in commit 951769cf12 threads: prefix log messages with thread names

Could add {}, according to developer notes for new code?

jamesob commented at 10:47 pm on April 26, 2019:

Done, thanks.

MarcoFalke approved

MarcoFalke commented at 10:22 pm on April 26, 2019: member

utACK 043a387639

Just some style comments that should probably be ignored

jamesob force-pushed on Apr 26, 2019

jamesob commented at 0:27 am on April 27, 2019: member

Latest benchmark; 0.7% difference seems attributable to noise.

ibd local 500000 504000 dbcache=2048

Empact commented at 8:22 am on April 27, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15849/commits/0cfcb8d470bd23f34e1bd81810444a1db8e4c702

nit: if I’m not mistaken you can use rvalues and move to avoid copies of the name throughout (channeling @sipa) #13168 (review)

 0diff --git a/src/util/threadnames.cpp b/src/util/threadnames.cpp
 1index 3217855f7..2ca91613c 100644
 2--- a/src/util/threadnames.cpp
 3+++ b/src/util/threadnames.cpp
 4@@ -40,18 +40,18 @@ static thread_local std::string g_thread_name;
 5 const std::string& util::ThreadGetInternalName() { return g_thread_name; }
 6 //! Set the in-memory internal name for this thread. Does not affect the process
 7 //! name.
 8-static void SetInternalName(const std::string& name) { g_thread_name = name; }
 9+static void SetInternalName(std::string&& name) { g_thread_name = name; }
10 
11 // Without thread_local available, don't handle internal name at all.
12 #else
13 
14 static const std::string empty_string;
15 const std::string& util::ThreadGetInternalName() { return empty_string; }
16-static void SetInternalName(const std::string& name) { }
17+static void SetInternalName(std::string&& name) { }
18 #endif
19 
20-void util::ThreadRename(const std::string& name)
21+void util::ThreadRename(std::string&& name)
22 {
23     SetThreadName(("bitcoin-" + name).c_str());
24-    SetInternalName(name);
25+    SetInternalName(std::move(name));
26 }
27diff --git a/src/util/threadnames.h b/src/util/threadnames.h
28index ecf6c50cc..aaf07b9bf 100644
29--- a/src/util/threadnames.h
30+++ b/src/util/threadnames.h
31@@ -10,7 +10,7 @@
32 namespace util {
33 //! Rename a thread both in terms of an internal (in-memory) name as well
34 //! as its system thread name.
35-void ThreadRename(const std::string&);
36+void ThreadRename(std::string&&);
37 
38 //! Get the thread's internal (in-memory) name; used e.g. for identification in
39 //! logging.

jamesob force-pushed on Apr 27, 2019

jamesob commented at 12:31 pm on April 27, 2019: member

@Empact thanks, pushed.

practicalswift commented at 4:49 pm on April 27, 2019: contributor

I’m not sure the change from const std::string& to std::string&& is an improvement TBH.

It brings no performance benefit in this case AFAICT and const std::string& is what we’re using in the rest of the code base.

0$ git grep -E 'const std::string( |)&' | wc -l
1851
2$ git grep -E 'std::string( |)&&' | wc -l
30

Empact commented at 9:10 pm on April 27, 2019: member

@practicalswift Rvalues make sense in cases where you have setters or constructors which can consume / incorporate the object passed in. We’re not using it with string but you can see other uses here:

0$ git grep -E '[a-z]&&' | wc -l
144

If you’re not seeing a performance benefit, it could be because my patch is apparently missing one move, as per: https://www.chromium.org/rvalue-references#TOC-9.-Move-constructors-should-be-accompanied-by-move-assignment-operators.

0-static void SetInternalName(std::string&& name) { g_thread_name = name; }
1+static void SetInternalName(std::string&& name) { g_thread_name = std::move(name); }

I don’t think this is critical, but it seems semantically appropriate.

in doc/release-notes-pr13168.md:4 in 4b4bc0067b outdated

0@@ -0,0 +1,6 @@
1+Thread names in logs
2+--------------------
3+
4+On platforms supporting `thread_local`, log lines are now prefixed by default

ryanofsky commented at 3:37 pm on April 29, 2019:

In commit “threads: prefix log messages with thread names” (62ea22c1c0bd5e41715f05ec893f2237ed3ad3b3)

This seems outdated (they aren’t prefixed by default).

jamesob commented at 5:34 pm on April 29, 2019:

Argh, another artifact of a bad rebase. Thanks.

ryanofsky approved

ryanofsky commented at 4:03 pm on April 29, 2019: member

utACK 4b4bc0067b41efaaff01abdaa8974aa7b88c132c. Only changes since last review are disabling by default and adding const and &&.

re: #15849 (comment)

I’m not sure the change from const std::string& to std::string&& is an improvement TBH.

I also wouldn’t consider this an improvement because now you can’t call SetInternalName with an lvalue (plain variable or reference). This is now an error:

0std::string name = "something";
1SetInternalName(name);

People get really confused about this stuff, but if the goal is to just move-enable SetInternalName, it should take plain std::string not const std::string&, and not std::string&&.

If a function just needs to read an argument (and not manipulate it before throwing it away or move it into a data structure) it should take const T&.
If a function wants to give callers the option of moving from an argument but still allow copies, it should take plain T.
If a function wants to force callers to move it should take T &&.

in src/sync.cpp:43 in 4b4bc0067b outdated

45-        sourceFile = pszFile;
46-        sourceLine = nLine;
47-        fTry = fTryIn;
48-    }
49+    CLockLocation(
50+        const char* pszName, const char* pszFile, int nLine, bool fTryIn, const std::string& thread_name) :

MarcoFalke commented at 4:54 pm on April 29, 2019:

style-nit (since you’d have to fix up other things anyway): Could move the colon to the next line, according to the style-guideline for new code?

0        const char* pszName, const char* pszFile, int nLine, bool fTryIn, const std::string& thread_name)
1            : fTry...

threads: introduce util/threadnames, refactor thread naming

This work is prerequisite to attaching thread names to log lines and deadlock
debug utilities. This code allows setting of an "internal" threadname per
thread on platforms where thread_local is available.

This commit also moves RenameThread() out of a more general module and adds a
numeric suffix to disambiguate between threads with the same name. It
explicitly names a few main threads using the new util::ThreadRename().

ae5f2b6a6c

tests: add threadutil tests ddd95ccb80

threads: prefix log messages with thread names

Introduce a new flag (`-logthreadnames`) which allows toggling
of this behavior.

383b186c28

threads: add thread names to deadlock debugging message

Also refactor CLockLocation to use an initialization list.

8722e54e56

in src/util/threadnames.cpp:43 in 4b4bc0067b outdated

38+
39+static thread_local std::string g_thread_name;
40+const std::string& util::ThreadGetInternalName() { return g_thread_name; }
41+//! Set the in-memory internal name for this thread. Does not affect the process
42+//! name.
43+static void SetInternalName(std::string&& name) { g_thread_name = name; }

MarcoFalke commented at 5:05 pm on April 29, 2019:

I’d prefer to undo this change. This does not give any performance gain, nor is it more readable

jamesob commented at 5:39 pm on April 29, 2019:

Yep, reverting.

jamesob commented at 5:40 pm on April 29, 2019:

FWIW all this bikeshedding is happening over a function call that is made ~20 times throughout the entire run of bitcoind.

jamesob force-pushed on Apr 29, 2019

jamesob commented at 5:58 pm on April 29, 2019: member

Thanks for the input, all. I’ve reverted back to Russ’ case 2 with a std::move addition. In any case it doesn’t matter much since this function is only called a handful of times during a bitcoind run.

Empact commented at 6:38 am on April 30, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15849/commits/8722e54e56fd959fd4ff2321b36a7640dee440c5

jnewbery commented at 6:44 pm on April 30, 2019: member

This is great. Very nicely structured PR and excellent commit messages 🙌

utACK 8722e54e56fd959fd4ff2321b36a7640dee440c5

The functional tests should be updated to have this enabled by default: https://github.com/bitcoin/bitcoin/blob/10ed4dff24470f22b4bc4030d534f4e1ed5f1c2f/test/functional/test_framework/test_node.py#L83

MarcoFalke commented at 7:24 pm on April 30, 2019: member

re-utACK 8722e54e56fd959fd4ff2321b36a7640dee440c5 (Only change since my previous review is DEFAULT_LOGTHREADNAMES=false and stylistic updates

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-utACK 8722e54e56fd959fd4ff2321b36a7640dee440c5 (Only change since my previous review is DEFAULT_LOGTHREADNAMES=false and stylistic updates
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgtewv9FxicQmO1+eKzOzgPMHm+KzOsWSRPTJVEtCtLgpSOpE2p8YAYCtxnQQEi
 8XI+7EzSSyEE9IL66WPwXiDVxG3RkZcCpgKcf/oIb5CaVxRN9Ob03uYNup9qCOAey
 9YZgg9GMkhbAxfOh5ZO3LbembFqMYBv1KqHnSuGSil2VAnaJdTFY6IMhhwR5SWvZs
101Peh510KITCYCg1doua9FcPUBB67yG3zqF36PlHUGivA5C+Zr9kLMNSdNbBzSXf5
11xlsqimIGSRjuk9qQx5P31XLx+yPEn3Si1ruTiN85CYMiPdRgIzg9lOmGHSBh1g38
12gV19YXiUjikxSWBndXKht9ensL6SPnxseD5Zsca1qAaKbcG+V6rkgE8sLEZro4Qn
13QShClrPnlwXvVmdJ8Ly5bnkyYWtC4jhlHu0bdtiyl/NOOWr29x6hGDsWw5l8tGfX
14anwN/XWxYIn4pC5La3LP/biD4Jd/vrp0kL1ENp3723OxvP/iiHh+6E/6EcD4stBE
15vv147CdK
16=4CZG
17-----END PGP SIGNATURE-----

Timestamp of file with hash 191793320e418666d66ac3696420980055c8e7f889470d345e05c3e4669b7398 -

pull[bot] referenced this in commit 2c35fe6238 on Apr 30, 2019

MarcoFalke merged this on Apr 30, 2019

MarcoFalke closed this on Apr 30, 2019

jnewbery removed this from the "Blockers" column in a project

in configure.ac:840 in 8722e54e56

837+        AC_MSG_RESULT(no)
838+        ;;
839+      *darwin*)
840+        # TODO enable thread_local on later versions of Darwin where it is
841+        # supported (per https://stackoverflow.com/a/29929949)
842+        AC_MSG_RESULT(no)

jonasschnelli commented at 8:58 am on May 2, 2019:

Why can’t we check with something like:

 0AC_MSG_CHECKING([for thread_local support])
 1AC_LINK_IFELSE([AC_LANG_SOURCE([
 2  #include <thread>
 3  static thread_local int foo = 0;
 4  static void run_thread() { foo++;}
 5  int main(){
 6  for(int i = 0; i < 10; i++) { std::thread(run_thread).detach();}
 7  return foo;
 8  }
 9  ])],
10  [
11    AC_DEFINE(HAVE_THREAD_LOCAL,1,[Define if thread_local is supported.])
12    AC_MSG_RESULT(yes)
13  ],
14  [
15    AC_MSG_RESULT(no)
16  ]
17)

jamesob commented at 6:12 pm on May 2, 2019:

Because bad thread_local behavior sometimes doesn’t fail explicitly on certain platforms (e.g. mingw32) until execution, and only under certain circumstances.

jonasschnelli commented at 6:19 pm on May 2, 2019:

Can you elaborate a little bit what “bad behavior” exactly mean? I assume the AC_LINK_IFELSE test would work on OSX,.. right? Or would it be a problem for our depends OSX build where we build with SDK 10.11 but allow min os version of 10.10?

Maybe I just open a PR and test it a bit.

MarcoFalke commented at 6:24 pm on May 2, 2019:

Yeah, I couldn’t add thread_local to the tests because of issues with macos: #14985

Might be solved by your AC_LINK check, but disabling it seems safer than running into issues later on.

jamesob commented at 6:00 pm on May 3, 2019:

@jonasschnelli see the gist I link to in the code comment above (https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605). This example compiles but stackoverflows when run, and I had to specially construct that example IIRC because trivial usages worked okay.

deadalnix referenced this in commit 3372a6e876 on Mar 24, 2020

jasonbcox referenced this in commit e409be178e on Mar 27, 2020

ftrader referenced this in commit 2ac5ab36d5 on Dec 1, 2020

kittywhiskers referenced this in commit efe677aab5 on Jun 5, 2021

kittywhiskers referenced this in commit 3b3697c989 on Jun 6, 2021

kittywhiskers referenced this in commit d4bd63c6bc on Jun 6, 2021

kittywhiskers referenced this in commit fece1d4c9b on Jun 7, 2021

kittywhiskers referenced this in commit f11a8727a7 on Jun 8, 2021

kittywhiskers referenced this in commit 98dc82cf17 on Jun 8, 2021

kittywhiskers referenced this in commit b0a21b7161 on Jun 9, 2021

kittywhiskers referenced this in commit 2cf83061bb on Jun 10, 2021

kittywhiskers referenced this in commit 67b80be769 on Jun 11, 2021

kittywhiskers referenced this in commit 111df62388 on Jun 16, 2021

kittywhiskers referenced this in commit dadaafd0cc on Jun 16, 2021

kittywhiskers referenced this in commit 187f473c64 on Jun 24, 2021

kittywhiskers referenced this in commit 3f8caf00bc on Jun 25, 2021

UdjinM6 referenced this in commit c5cc285d0e on Jun 26, 2021

DrahtBot locked this on Dec 16, 2021

Thread names in logs and deadlock debug tools #15849

threadnames off (default)

2018-05-threadnames.3 vs. master (absolute)

2018-05-threadnames.3 vs. master (relative)

threadnames on

2018-05-threadnames-take-2 vs. master (absolute)

2018-05-threadnames-take-2 vs. master (relative)

Conflicts

absolute

relative