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 <[email protected]>
|
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 <[email protected]>
| // 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 |
Signed-off-by: Alyssa Wilk <[email protected]>
jmarantz
left a comment
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 <[email protected]>
* 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 <[email protected]>
…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 <[email protected]>
* 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 <[email protected]>
4 participants
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