Thread names in logs and deadlock debug tools #15849
Conversation
16f551c to
a0ca62b
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
a0ca62b to
503c1ff
Compare
src/threadutil.cpp
Outdated
| //! internal name. | ||
| static void SetThreadName(const char* name) | ||
| { | ||
| #if defined(PR_SET_NAME) |
There was a problem hiding this comment.
In commit "threads: introduce threadutil, refactor thread naming" (91cc480)
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)
src/logging.cpp
Outdated
| @@ -5,6 +5,9 @@ | |||
|
|
|||
| #include <logging.h> | |||
| #include <util/time.h> | |||
| #include <threadutil.h> | |||
There was a problem hiding this comment.
In commit "threads: prefix log messages with thread names" (4fba3ea)
Could sort alphabetically (thread before util)
Note that this doesn't affect anything unless DEBUG_LOCKCONTENTION is defined. See discussions here: - bitcoin#11722 (review) - bitcoin#13168 (comment)
503c1ff to
28198a7
Compare
|
@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. |
|
Reran benchmarking; no performance difference. absolute
relative
|
28198a7 to
2e53d90
Compare
2e53d90 to
eed4a5c
Compare
|
@ryanofsky Oops! Thanks, pushed. |
There was a problem hiding this comment.
utACK eed4a5c. 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).
eed4a5c to
043a387
Compare
043a387 to
0cfcb8d
Compare
|
Latest benchmark; 0.7% difference seems attributable to noise. |
|
utACK 0cfcb8d nit: if I'm not mistaken you can use rvalues and move to avoid copies of the name throughout (channeling @sipa) #13168 (comment) diff --git a/src/util/threadnames.cpp b/src/util/threadnames.cpp
index 3217855f7..2ca91613c 100644
--- a/src/util/threadnames.cpp
+++ b/src/util/threadnames.cpp
@@ -40,18 +40,18 @@ static thread_local std::string g_thread_name;
const std::string& util::ThreadGetInternalName() { return g_thread_name; }
//! Set the in-memory internal name for this thread. Does not affect the process
//! name.
-static void SetInternalName(const std::string& name) { g_thread_name = name; }
+static void SetInternalName(std::string&& name) { g_thread_name = name; }
// Without thread_local available, don't handle internal name at all.
#else
static const std::string empty_string;
const std::string& util::ThreadGetInternalName() { return empty_string; }
-static void SetInternalName(const std::string& name) { }
+static void SetInternalName(std::string&& name) { }
#endif
-void util::ThreadRename(const std::string& name)
+void util::ThreadRename(std::string&& name)
{
SetThreadName(("bitcoin-" + name).c_str());
- SetInternalName(name);
+ SetInternalName(std::move(name));
}
diff --git a/src/util/threadnames.h b/src/util/threadnames.h
index ecf6c50cc..aaf07b9bf 100644
--- a/src/util/threadnames.h
+++ b/src/util/threadnames.h
@@ -10,7 +10,7 @@
namespace util {
//! Rename a thread both in terms of an internal (in-memory) name as well
//! as its system thread name.
-void ThreadRename(const std::string&);
+void ThreadRename(std::string&&);
//! Get the thread's internal (in-memory) name; used e.g. for identification in
//! logging. |
0cfcb8d to
4b4bc00
Compare
|
@Empact thanks, pushed. |
|
I'm not sure the change from It brings no performance benefit in this case AFAICT and |
|
@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: 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. -static void SetInternalName(std::string&& name) { g_thread_name = name; }
+static void SetInternalName(std::string&& name) { g_thread_name = std::move(name); }I don't think this is critical, but it seems semantically appropriate. |
There was a problem hiding this comment.
utACK 4b4bc00. 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:
std::string name = "something";
SetInternalName(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 &&.
|
This is great. Very nicely structured PR and excellent commit messages 🙌 utACK 8722e54 The functional tests should be updated to have this enabled by default: |
|
re-utACK 8722e54 (Only change since my previous review is DEFAULT_LOGTHREADNAMES=false and stylistic updates Show signature and timestampSignature: Timestamp of file with hash |
8722e54 threads: add thread names to deadlock debugging message (James O'Beirne) 383b186 threads: prefix log messages with thread names (James O'Beirne) ddd95cc tests: add threadutil tests (James O'Beirne) ae5f2b6 threads: introduce util/threadnames, refactor thread naming (James O'Beirne) 188ca75 disable HAVE_THREAD_LOCAL on unreliable platforms (James O'Beirne) Pull request description: I'm resurrecting this one (from bitcoin#13168) because I need it to make progress on bitcoin#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 | ``` ACKs for commit 8722e5: Empact: utACK bitcoin@8722e54 jnewbery: utACK 8722e54 MarcoFalke: re-utACK 8722e54 (Only change since my previous review is DEFAULT_LOGTHREADNAMES=false and stylistic updates Tree-SHA512: 50af992708295b8d680cf10025262dd964e599a356bdfc1dfc84fb18c00afabcb34d3d12d551b0677ff81f8fccad0e17c1d5b24dfecb953a913bc77fdd1a4577
| *darwin*) | ||
| # TODO enable thread_local on later versions of Darwin where it is | ||
| # supported (per https://stackoverflow.com/a/29929949) | ||
| AC_MSG_RESULT(no) |
There was a problem hiding this comment.
Why can't we check with something like:
AC_MSG_CHECKING([for thread_local support])
AC_LINK_IFELSE([AC_LANG_SOURCE([
#include <thread>
static thread_local int foo = 0;
static void run_thread() { foo++;}
int main(){
for(int i = 0; i < 10; i++) { std::thread(run_thread).detach();}
return foo;
}
])],
[
AC_DEFINE(HAVE_THREAD_LOCAL,1,[Define if thread_local is supported.])
AC_MSG_RESULT(yes)
],
[
AC_MSG_RESULT(no)
]
)There was a problem hiding this comment.
Because bad thread_local behavior sometimes doesn't fail explicitly on certain platforms (e.g. mingw32) until execution, and only under certain circumstances.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
Summary: Backport of core [[bitcoin/bitcoin#15849 | PR15849]]. The first commit disables `thread_local` on some platforms and is not ported: - The Darwin exclusion is no longer needed since D5514. - The MinGw exclusion does not seem necessary, at least with our version. The tests run fine on Windows, both 32 and 64 bits. Also see D2219 which removes the `thread_local` check. This introduces a number of bugs: - This is breaking the build for some *BSD. - This makes some Unix tools inefficient, like `ps` or `killall`. - Some thread names are truncated and cannot be differentiated. The fixes will be ported in their own diff for readability. Test Plan: ninja check make check Run the Gitian builds. Run test_bitcoin.exe on Windows (32 bits) for sanity. Builld and run the tests on OSX. Start `bitcoind` with `-logthreadnames=1` and check the debug log contains the thread names. Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5540
Summary: This is a follow-up for D5540. Despite the tests passing, the diff is causing bitcoin-qt to fail quickly. Disabling thread_local for this compiler fixes the issue. The code is extracted from [[bitcoin/bitcoin#15849 | PR15849]], but the tests for thread_local is a simplified version. Test Plan: Build for win32 with autotools and cmake. Run bitcoin-qt and check it doesn't crash. As a sanity check, build for win64 and any linux and check `thread_local` is enabled. Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5587
Thread names in logs and deadlock debug tools See merge request bitcoin-cash-node/bitcoin-cash-node!834 This is a backport of [PR15849](bitcoin/bitcoin#15849) (Thread names in logs and deadlock debug tools). See original PR for benchmarks. Also includes some smaller follow-up PR's: * [PR15968](bitcoin/bitcoin#15968) - Fix portability issue with pthreads * [PR16984](bitcoin/bitcoin#16984) - util: Make thread names shorter * [PR17038](bitcoin/bitcoin#17038) - Don't rename main thread at process level ## Test plan New tests: `ninja check` Run with `-logthreadnames`, observe lines are prefixed with thread ``` 2020-10-28T11:39:16Z [init] init message: Done loading 2020-10-28T11:39:16Z [opencon] opencon thread start 2020-10-28T11:39:16Z [dnsseed] 0 addresses found from DNS seeds 2020-10-28T11:39:16Z [msghand] msghand thread start 2020-10-28T11:39:16Z [dnsseed] dnsseed thread exit ``` Compile with lockorder debugging enabled: `cmake -GNinja -DCMAKE_BUILD_TYPE=Debug ..`
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
partial bitcoin#15842, merge bitcoin#15849, bitcoin#17342, bitcoin#18710: Add local thread pool to CCheckQueue
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)
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)