Switch ThreadId to an opaque value type. #7330

fowles · 2019-06-19T22:08:51Z

Description: ThreadId does not need to be an abstract class and that just interferes
with using them as keys in hashtables.

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes: N/A

jmarantz · 2019-06-19T23:00:14Z

Actually it does need to be an abstract type. Please read #6825 (comment)

And it's not difficult to make them hashable. Just define a pure hash() method and supply a posix impl.

I don't think it would be hard to revive/replicate #6825 and just make it work. Can we go in that direction instead of concretizing it. Happy to help if there is trouble.

jmarantz · 2019-06-19T23:03:12Z

Oh I just correlated who you are ... is there some reason you wanted to go down this route rather than just make the object hashable?

fowles · 2019-06-20T02:27:41Z

The idea of a full virtual hierachy for something that is an int64 (or smaller) on all platforms seems wasteful. Also, it has harder than just adding a hash virtual, you will need custom Hasher and Eq operators too. Making this a value makes it that much simpler to work with.

fowles · 2019-06-20T02:34:34Z

Also, this is not a pthread_id it is a https://linux.die.net/man/2/gettid or a http://www.manpagez.com/man/3/pthread_threadid_np/ both of which are == comparable if I am reading it correctly...

jmarantz · 2019-06-20T03:17:53Z

Current impl in posix is in source/common/common/posix/thread_impl.cc and is:

int64_t getCurrentThreadId() {
#ifdef __linux__
  return static_cast<int64_t>(syscall(SYS_gettid));
#elif defined(__APPLE__)
  uint64_t tid;
  pthread_threadid_np(nullptr, &tid);
  return tid;
#else
#error "Enable and test pthread id retrieval code for you arch in pthread/thread_impl.cc"
#endif
}

So the linux version calls syscall(SYS_gettid)) .

Doc for gettid() I found is https://linux.die.net/man/2/gettid : which is pid_t gettid(void); and says:

The thread ID returned by this call is not the same thing as a POSIX thread ID (i.e., the opaque value returned by pthread_self(3)).

So that's where I got to pid_t from. I don't know of a platform where you couldn't render the pid_t as a uint64_t but the doc seems specific that you ought not to assume that, if you want your code to be portable. There were some efforts a while back to get Envoy working on Windows, though they seem to be dormant at the moment.

My feeling is that it's not maximally efficient to have to have a virtual class around a uint64_t but given Envoy's thread-per-core model (https://blog.envoyproxy.io/envoy-threading-model-a8d44b922310), there may be hundreds but not thousands of threads, so this is not going to amount to any measurable cost in memory consumption, and it might make porting life easier for someone.

RE Hash and Eq functors; I don't think you need an Eq functor if we add operator== to the class; it just works....realizing now who I'm talking to; is supplying a Hash but not an Eq to absl::flat_hash_map -- is that something that currently works but is not really supported? Obviously it's not hard to work around if we need to specify both.

fowles · 2019-06-20T03:24:16Z

If you want a table of the thing you will need to do:

struct Hash {
  size_t operator()(const ThreadIdPtr& p) const {
    return absl::Hash<ThreadId>{}(*p);
  }
};

struct Eq {
  bool operator()(const ThreadIdPtr& lhs, const ThreadIdPtr& rhs) const {
     // down cast and compare
  }
};

absl::flat_hash_map<ThreadIdPtr, V, Hash, Eq> m;

you definitely don't want the default Eq because it will do pointer comparison for that case

fowles · 2019-06-20T03:26:59Z

I am a bit confused, #6825 (comment) specifically is talking about https://linux.die.net/man/3/pthread_self which is different from what the implementation you quoted use. My point is the implementation doesn't use the thing you linked in that comment and uses something that is == comparable (I think)

jmarantz · 2019-06-20T03:29:36Z

Good point; I was thinking about keeping a table of the objects instead of pointers to them, but that's hard since ThreadId is abstract :) OK we need the Eq functor too.

fowles · 2019-06-20T03:34:40Z

I am not wed to this change, but the extra indirections felt easy to remove and the result is an Id that you could keep by value in tables and have everything be much cleaner for.

jmarantz · 2019-06-20T03:41:13Z

I think you're right; I was confused about pthread_self being relevant here; it looks like in posix pid_t is defined as a signed integer type, and in Windows it's a DWORD.

One minor annoyance is that if we change the API we'll need to change the windows impl in source/common/common/win32/thread_impl.cc and I don't know who's set up to test that, and it's not in CI. But I guess we could just break it and let people who work on Windows sort it out later until they can get into CI. @sesmith177 are you still looking at Windows stuff? Last couple of times I pinged you you didn't respond.

With regards to this PR you'll have to sort out DCO: https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#dco-sign-your-work as well as fixing the format errors from CI and the compile error in compile_time_options.

fowles · 2019-06-20T03:44:46Z

I will sort out those details tomorrow! Cheers

sesmith177 · 2019-06-20T13:46:36Z

@jmarantz yeah, I left Pivotal a couple months ago and so am no longer working on the port of Envoy to Windows.

Since a DWORD is an unsigned 32-bit integer, it may be OK to just store it an int64, but it may just be safer to typedef it in include/envoy/thread/thread.h or something

fowles · 2019-06-20T13:51:47Z

The static_cast<int64_t> for the DWORD is pretty easy and guaranteed to be safe. The sentinel I am using is out of range for a DWORD and thus guaranteed to be safe. I am less clear on if INT_MIN is safe sentinel for posix systems, but my guess is yes ;)

jmarantz · 2019-06-20T13:54:30Z

I raised an issue on the slack (envoy-dev) about what to do about the windows code maintenance:

Hi all -- it looks like there is no one working currently on Envoy for Windows, but there's a lot of support code in there which is untestable by (AFAIK) anyone currently working in the codebase. What do folks think we should do with it...I'm left sometimes speculating on the windows portability implications of various PRs.

ThreadId does not need to be an abstract class and that just interferes with using them as keys in hashtables. Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fowles · 2019-06-20T18:44:57Z

fixed the formatting and DCO stuff

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

jmarantz

generally looks fine; just wanted to see a hash-table definition come out of this effort as well :)

Also I will + in an Envoy Windows developer (Matt Horan) once he gives me his github ID, which I could not figure out.

include/envoy/thread/thread.h

source/common/singleton/manager_impl.cc

source/common/common/win32/thread_impl.cc

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

wrowe · 2019-06-26T14:17:42Z

Hi there, note that on master I'm blocked on issue 7357 to have Win32 compile, so if someone has recommendations for a clean fix to the use of absl against an older stdc++ style str implementation, that should jumpstart my ability to inspect this fix.

jmarantz · 2019-06-26T14:26:15Z

@wrowe @mhoran does it make sense to proceed with merging this PR with the somewhat speculative win32 implementation, and leave it in your hands to clean it up as we get win32 into CI?

@fowles your clang-tidy errors appear to be in windows-related files where clang can't find windows.h etc. @lizan how do we work around this?

The other CI failures may be known flakes and we can trigger a retest. I still need to do a final pass over the code. Thanks for pushing through this Matt.

/retest

repokitteh-read-only · 2019-06-26T14:26:20Z

🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7330 (comment) was created by @jmarantz.

see: more, trace.

jmarantz · 2019-06-26T14:26:53Z

/azp run

jmarantz · 2019-06-27T01:06:13Z

@envoyproxy/senior-maintainers over to you for a final pass.

alyssawilk · 2019-06-27T13:35:05Z

@fowles looks like you need one more clang tidy pass?
regarding maintainers arguably we're supposed to have a non-googler. @lizan would you be up for taking this one?

jmarantz · 2019-06-27T13:38:17Z

I don't think clang-tidy can be fixed for this PR as the PR includes windows-specific files, for which clang-tidy cannot find includes.

jmarantz · 2019-06-27T13:39:31Z

Actually he can't fix the errors but he could fix the warnings:

/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/source/common/common/posix/thread_impl.cc:27:34: warning: pass by value and use std::move [modernize-pass-by-value]
ThreadImplPosix::ThreadImplPosix(std::function<void()> thread_routine)
                                 ^
/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/source/common/common/posix/thread_impl.cc:28:23: warning: parameter 'thread_routine' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
    : thread_routine_(thread_routine) {
                      ^
                      std::move(    )
/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/source/server/watchdog_impl.h:23:20: warning: std::move of the variable 'thread_id' of the trivially-copyable type 'Thread::ThreadId' has no effect; remove std::move() [performance-move-const-arg]
      : thread_id_(std::move(thread_id)), time_source_(tsource),
                   ^~~~~~~~~~         ~
/build/tmp/_bazel_bazel/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/test/mocks/server/mocks.h:183:3: warning: annotate this function with 'override' or (rarely) 'final' [modernize-use-override]
  ~MockGuardDog();
  ^

alyssawilk · 2019-06-27T13:40:54Z

Ah, that's what I get for commenting without reading the full thread.
Well possibly @lizan can help with that too ;-)
We should keep an eye on how often that happens - it's the first I've seen and we obviously don't want to get in a situation where Matt/Harvey have to regularly force-merge because of over-enthusiastic tidy

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fowles · 2019-06-27T18:35:57Z

fixed clang-tidy warnings and pulled from head

alyssawilk · 2019-07-01T14:34:07Z

Ah, lizan is out this week. Well as it's going to require Matt's force-merge anyway, @mattklein123 can you take a look?

mattklein123

Thanks, looks great with some tiny nits.

/wait

include/envoy/thread/thread.h

mattklein123 · 2019-07-01T15:37:05Z

include/envoy/thread/thread.h

+  }
+
+private:
+  int64_t id_;


I think we don't want this. As it stands, this is value type that can be copied and assigned normally. If we make this const we lost assignment

source/common/singleton/manager_impl.h

source/server/watchdog_impl.h

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fowles · 2019-07-01T17:19:41Z

Pulled from latest mainline, merged, made most of the changes requested

mattklein123

Thank you!

mattklein123 · 2019-07-01T19:37:41Z

Sorry can you fix format?

/wait

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fowles · 2019-07-01T20:25:47Z

format fixed

mattklein123

Thank you!

mattklein123 assigned jmarantz Jun 19, 2019

fowles added 2 commits June 20, 2019 14:34

Switch ThreadId to an opaque value type.

f604a10

ThreadId does not need to be an abstract class and that just interferes with using them as keys in hashtables. Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fix formatting

5e84438

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

add missing include

50a4db2

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

jmarantz reviewed Jun 22, 2019

View reviewed changes

include/envoy/thread/thread.h Show resolved Hide resolved

source/common/singleton/manager_impl.cc Outdated Show resolved Hide resolved

jmarantz reviewed Jun 24, 2019

View reviewed changes

source/common/common/win32/thread_impl.cc Show resolved Hide resolved

fowles added 3 commits June 24, 2019 13:16

Add absl::Hash specialization

237d1f4

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

Push ThreadFactory through ManagerImpl

0b18676

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fix formatting

7f149e5

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fowles requested review from htuch and mattklein123 as code owners June 25, 2019 17:15

fowles added 2 commits June 27, 2019 14:35

Fix clang-tidy warnings

080960e

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

Merge branch 'master' of github.com:envoyproxy/envoy

230d2ed

fowles dismissed jmarantz’s stale review via 230d2ed June 27, 2019 18:35

jmarantz previously approved these changes Jun 27, 2019

View reviewed changes

mattklein123 self-assigned this Jul 1, 2019

mattklein123 requested changes Jul 1, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 1, 2019

add requested const

8b05244

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fowles dismissed jmarantz’s stale review via 8b05244 July 1, 2019 17:18

repokitteh-read-only bot removed the waiting label Jul 1, 2019

Merge branch 'master' of github.com:envoyproxy/envoy

edb865d

mattklein123 previously approved these changes Jul 1, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 1, 2019

fix include order

4adba79

Signed-off-by: Matt Kulukundis <matt.fowles@gmail.com>

fowles dismissed mattklein123’s stale review via 4adba79 July 1, 2019 20:25

repokitteh-read-only bot removed the waiting label Jul 1, 2019

mattklein123 approved these changes Jul 1, 2019

View reviewed changes

mattklein123 merged commit 7d5e6b2 into envoyproxy:master Jul 1, 2019

ipuustin mentioned this pull request Jul 16, 2019

dispatcher: fix a comment. #7597

Merged

jmarantz mentioned this pull request Sep 8, 2019

absl: Absl hash hook in a couple of places rather than hash functors #8179

Merged

Switch ThreadId to an opaque value type. #7330

Switch ThreadId to an opaque value type. #7330

Conversation

fowles commented Jun 19, 2019

jmarantz commented Jun 19, 2019

jmarantz commented Jun 19, 2019

fowles commented Jun 20, 2019

fowles commented Jun 20, 2019

jmarantz commented Jun 20, 2019 • edited Loading

fowles commented Jun 20, 2019

fowles commented Jun 20, 2019

jmarantz commented Jun 20, 2019

fowles commented Jun 20, 2019

jmarantz commented Jun 20, 2019

fowles commented Jun 20, 2019

sesmith177 commented Jun 20, 2019

fowles commented Jun 20, 2019

jmarantz commented Jun 20, 2019 • edited Loading

fowles commented Jun 20, 2019

jmarantz left a comment

Choose a reason for hiding this comment

wrowe commented Jun 26, 2019

jmarantz commented Jun 26, 2019

repokitteh-read-only bot commented Jun 26, 2019

jmarantz commented Jun 26, 2019

jmarantz commented Jun 27, 2019

alyssawilk commented Jun 27, 2019

jmarantz commented Jun 27, 2019

jmarantz commented Jun 27, 2019

alyssawilk commented Jun 27, 2019

fowles commented Jun 27, 2019

alyssawilk commented Jul 1, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Jul 1, 2019

Choose a reason for hiding this comment

fowles Jul 1, 2019

Choose a reason for hiding this comment

fowles commented Jul 1, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 commented Jul 1, 2019

fowles commented Jul 1, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

jmarantz commented Jun 20, 2019 •

edited

Loading

jmarantz commented Jun 20, 2019 •

edited

Loading