threads: allowing for multiple main threads for multi-envoy use case#21277
threads: allowing for multiple main threads for multi-envoy use case#21277alyssawilk merged 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
cc @jpsim an alternate to envoyproxy/envoy-mobile#2268 |
source/common/common/thread.cc
Outdated
| mutable absl::Mutex mutex_; | ||
|
|
||
| std::atomic<uint32_t> skip_asserts_{}; | ||
| absl::flat_hash_map<std::thread::id, uint32_t> main_threads_to_usage_count_; |
There was a problem hiding this comment.
Should this have ABSL_GUARDED_BY(mutex_)?
|
Nice! Happy to approve after you've had a chance to consider my comment. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| // result in the correct result even without a lock. | ||
| return std::this_thread::get_id() == main_thread_id_; | ||
| absl::MutexLock lock(&mutex_); | ||
| return main_threads_to_usage_count_.find(std::this_thread::get_id()) != |
There was a problem hiding this comment.
When I originally added this @htuch was concerned that a mutex-lock while checking, even under an ASSERT, might introduce serialization in tests run with ASSERTs enabled, and reduce the chance of hitting unrelated races.
But looking at the code I don't see this as a big problem -- the lock is only held during a single hash lookup.
OTOH it looks very easy to use a thread_local bool here to do the inMainThread check. Any reason not to do that?
There was a problem hiding this comment.
I'm wasn't worried about major contention blocking causing performance distortion (I think), but probably just differences in timing when there is some correlated access. If there is no easy way to do this and remote singleton, let's go with this PR, but if thread_local works I agree it might be a bit better.
|
/wait |
jmarantz
left a comment
There was a problem hiding this comment.
using a thread_local bool is not required to get this in; could be a follow-up if we feel like it.
|
@jpsim if you bump main on your branch it should pass. If not ping me back! |
|
I'll let you know, thanks everyone! |
Pulls in envoyproxy/envoy#21277 to unblock further progress on multi-engine support. Signed-off-by: JP Simard <jp@jpsim.com>
* Pulls in envoyproxy/envoy#21277 to unblock further progress on multi-engine support * Updates Docker build image to the same version used in Envoy * Updates LLVM to 14.0.0 * Updates Python to 3.10 Diff: envoyproxy/envoy@efbbb04...d88f31b Signed-off-by: JP Simard <jp@jpsim.com>
…nvoyproxy#21277) Risk Level: low Testing: turned off assertions in multi-envoy test Docs Changes: n/a Release Notes: n/a part of envoyproxy/envoy-mobile#332 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
* Pulls in #21277 to unblock further progress on multi-engine support * Updates Docker build image to the same version used in Envoy * Updates LLVM to 14.0.0 * Updates Python to 3.10 Diff: efbbb04...d88f31b Signed-off-by: JP Simard <jp@jpsim.com>
Risk Level: low
Testing: turned off assertions in multi-envoy test
Docs Changes: n/a
Release Notes: n/a
part of envoyproxy/envoy-mobile#332