thread: add new optional Thread::Options argument to thread creation, which initially will allow specification of a name.

[jmarantz]

Commit Message: Currently Envoy threads are unnamed. When tuning performance it might be useful to be able set names for threads based on what they are used for. Later it might be desirable to cpu affinity or other attributes when creating threads, but for now just adding name visibility seems useful.
Additional Description: Also moved a couple of platform-specific classes out of an impl.h and into the impl.cc as they weren't referenced anywhere else.
Risk Level: medium -- this does add an absl::Notification to thread creation to avoid a race when setting thread names, which affects prod. Also, this PR modifies Windows-specific code without manually testing it or even compiling it.
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Fixes #11437

[wrowe]

This is what you'll observe compiling source/common/filesystem/win32/watcher_impl.cc

source/common/filesystem/win32/watcher_impl.cc(34): error C2065: 'dispatcher_': undeclared identifier
source/common/filesystem/win32/watcher_impl.cc(91): error C2039: 'ThreadImplWin32': is not a member of 'Envoy::Thread'
bazel-out/x64_windows-opt/bin/source/common/common/_virtual_includes/thread_impl_lib_win32\common/common/thread_impl.h(9): note: see declaration of 'Envoy::Thread'
source/common/filesystem/win32/watcher_impl.cc(91): error C2061: syntax error: identifier 'ThreadImplWin32'

The problem is that from the dispatcher, we need the HANDLE hThread (which is -not- the thread ID) which roughly corresponds to posix pthread_t of that implementation. The thread ID isn't helpful to the QueueUserAPC call which stacks the trigger onto the corresponding thread of that watcher.

watcher_impl used to consume the ThreadImplWin32 which provided transparent access to that thread handle, the new API will need to do the equivilant.

[wrowe]

cc @sunjayBhatia

[jmarantz]

Due to the rather restrictive posix limit I wanted to test we were actually getting the 15 bytes, and if we wind up on a platform that is limited further we need to learn about it in tests because we'll need to re-think how we name the dispatcher threads to make them useful.

Note it's not enabled in prod, only for debug/fastbuild.

[eziskind]

LGTM

[jmarantz]

Ah I managed to get windows CI passing :). Thanks @wrowe and @sunjayBhatia . Now for MacOS!

Of course MacOS fails for lots of people but this PR makes it fail worse, specifically because pthread_setname_np appears to not be available.

[jmarantz]

@envoyproxy/senior-maintainers I think this is ready for a look.

[ggreenway]

The extra #if seems unneeded. Can you make getNameFromOS() return a std::string, and check for equality in the ASSERT so that no extra #if is needed here?

[jmarantz]

it's a little tricky because it is valid for getNameFromOS to fail. That will happen if the thread exits after the call to pthread_setname_np and before the call to pthread_getname_np. This seemed like the easiest way to assert that, if we can get the name at all, it's what we expect.

If you have other ideas of how to structure this better I'm happy to try, but this seemed like the simplest approach so far.

[ggreenway]

Regardless of the ifdef, I'd say make it return an optional<std::string>. And then I think you can change it to ASSERT(!getNameFromOS().valid() || getNameFromOS() == name_)

[jmarantz]

yup that's better; actually i had done something similar to this but I'll switch to optional.

[jmarantz]

actually I like what I have better at both call-sites. I don't like calling getNameFromOS() twice as it can fail due to subprocess exiting between the two calls.

[jmarantz]

Any more comments on this? I'd love to get this in so it can start its way through the pipeline where we can use it to gain insight into how threading behaves on our traffic. Thanks!