Thoughts on pulling the TraceThread class out of util into the new files you're creating here? Seems like a good home for tracethread and would reduce the clutter in util

Concept ACK

@promag thanks for the review; I've pushed changes incorporating your feedback.

@skeees I like that idea but maybe we can save it for a future PR? That entails changing a bunch of call sites and I'm worried about introducing a circular dependency between util and threadnames (by way of logging).

How does the use of boost::thread::id interact with eventual boost::thread removal? IIRC there was something about no guarantees being made that you can interact with boost::thread if the thread was started by std, though it usually works in practice. We do have a HAVE_THREAD_LOCAL defined, which should be set everywhere but OSX, so maybe we could just turn it on there if boost::thread::id is an issue?

Concept ACK- just a comment on how the maps are used.

@TheBlueMatt good points, thanks for looking. I've pushed changes which

• only make use of boost::thread when thread_local is not available. When it is available, we assign each thread a thread_local-cached ID based on a static counter.
• incorporate @conscott's suggestion of using m_name_to_count which nicely cuts out a bunch of code.
• add tests.

(jamesob/threadnames.1 -> jamesob/threadnames.3)

This changeset is ready for review-for-merge IMO.

@theuni What's the status of thread_local on OSX? I remember seeing https://stackoverflow.com/questions/28094794/why-does-apple-clang-disallow-c11-thread-local-when-official-clang-supports, but which Xcode version's patches does our clang run with, or...?

I've pushed a small change removing GetProcessName support for *BSD systems. After double-checking pthread_getname support for OpenBSD, I found that it doesn't seem to exist. As a result, any threads which are not explicitly named with Rename() will be labeled "<unknown>" on BSD systems.

 $ diff -uw <(git diff master..threadnames.3) <(git diff master..threadnames.4)

--- /proc/self/fd/12    2018-05-01 14:56:59.561313752 -0400
+++ /proc/self/fd/13    2018-05-01 14:56:59.549313568 -0400
@@ -2215,10 +2215,10 @@
      void reset();
 diff --git a/src/threadnames.cpp b/src/threadnames.cpp
 new file mode 100644
-index 0000000..305c3b4
+index 0000000..92e3f75
 --- /dev/null
 +++ b/src/threadnames.cpp
-@@ -0,0 +1,135 @@
+@@ -0,0 +1,133 @@
 +// Copyright (c) 2018 The Bitcoin Core developers
 +// Distributed under the MIT software license, see the accompanying
 +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
@@ -2345,8 +2345,6 @@
 +
 +#if defined(PR_GET_NAME)
 +    ::prctl(PR_GET_NAME, pthreadname_buff);
-+#elif (defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__DragonFly__))
-+    pthread_get_name_np(pthread_self(), pthreadname_buff);
 +#elif defined(MAC_OSX)
 +    pthread_getname_np(pthread_self(), pthreadname_buff, sizeof(threadname_buff));
 +#else

After spending the day figuring out that thread_local is apparently broken on mingw32 compilers, I'm now falling back to boost thread IDs for WIN32. Tests are passing and this is once again ready for review.

Per @sipa's advice, I'm now using std::thread to determine thread IDs for all platforms instead of either boost or the ham-fisted thread_local code.

jamesob/threadnames.11 -> jamesob/threadnames.13

If platform-specific functions are being used anyway, why not just use pthread_getspecific/pthread_setspecific, which are meant for exactly this? That also makes it a trivial transition when we're finally ready to use thread_local.

@theuni what are you talking about replacing at this point? We're now platform agnostic with std::thread, and it seems to work fine with boost threads. Any reason you can see that it won't work?

@jamesob See the top two commits here for a quick attempt: https://github.com/theuni/bitcoin/commits/thread_names

That addresses a few things that make me hesitate about this PR:

• system thread id/name are conflated with internal use. This drags in system requirements (name length, for ex) and overhead for internal functions.
• Heavy map lookup with locking. logging: bypass timestamp formatting when not logging #12970 demonstrated how painful overhead in logging can be.
• Most can go away once we can use thread_local unconditionally.

I realize that my changes are oversimplified, it's just a quick POC for discussion.

@theuni I think your simple implementation looks great. Very much agree with your concern re: map lookup and locking overhead, and I think there's a way to combine our approaches to avoid it. Let me know if this sounds right:

IMO lock acquisition during the single Rename() call per thread is acceptable, but we want to avoid any subsequent locking during GetName() (since it is called per logline). So: during Rename() we can use Registry's count map for numeric suffix generation and then use your method for caching the name of the thread for use during GetName().

If that sounds good to you, I'll cherry-pick your two commits onto this branch and reorganize accordingly. Otherwise if you think the numeric suffix isn't worthwhile, I'm happy to close this out and proceed on your commits only.

@jamesob Looking again, I think handling the suffix at this layer is unnecessary and adds a significant amount of complication. Why not just add a suffix before passing the name into TraceThread() ?

@theuni ugh, you're right, that's dumb. Let's go off of your commits.

From IRC:

<cfields> jamesob: noo!
<cfields> jamesob: I hope you don't think I'm being NIH about that PR, that wasn't my intention at all. Coding up an alternative myself helps me to understand the challenges better, it wasn't meant to replace your work.