From a9e2ef199ec5129bf36830f8781754ae3d7be1a3 Mon Sep 17 00:00:00 2001 From: AJ Heller Date: Wed, 1 Feb 2023 17:10:14 -0800 Subject: [PATCH] [EventEngine] Skip legacy fork handling for ExecCtx when in an EventEngine thread (#32229) * [EventEngine] Implement EventEngine::IsWorkerThread() * lighter-weight thread pool check * add lightweight thread_local flag for EventEngine/iomgr fork * generate_projects; add files * fix * back out EE implementation changes * better description * fix --- BUILD | 1 + CMakeLists.txt | 1 + Makefile | 1 + build_autogenerated.yaml | 2 ++ config.m4 | 1 + config.w32 | 1 + gRPC-C++.podspec | 2 ++ gRPC-Core.podspec | 3 ++ grpc.gemspec | 2 ++ grpc.gyp | 1 + package.xml | 2 ++ src/core/BUILD | 8 ++++++ src/core/lib/event_engine/thread_local.cc | 29 +++++++++++++++++++ src/core/lib/event_engine/thread_local.h | 32 +++++++++++++++++++++ src/core/lib/event_engine/thread_pool.cc | 17 ++++++----- src/core/lib/event_engine/thread_pool.h | 3 ++ src/core/lib/gprpp/fork.cc | 7 +++++ src/python/grpcio/grpc_core_dependencies.py | 1 + tools/doxygen/Doxyfile.c++.internal | 2 ++ tools/doxygen/Doxyfile.core.internal | 2 ++ 20 files changed, 109 insertions(+), 9 deletions(-) create mode 100644 src/core/lib/event_engine/thread_local.cc create mode 100644 src/core/lib/event_engine/thread_local.h diff --git a/BUILD b/BUILD index e5220bf9b85..ae04a406d63 100644 --- a/BUILD +++ b/BUILD @@ -719,6 +719,7 @@ grpc_cc_library( "debug_location", "//src/core:construct_destruct", "//src/core:env", + "//src/core:event_engine_thread_local", "//src/core:examine_stack", "//src/core:gpr_atm", "//src/core:no_destruct", diff --git a/CMakeLists.txt b/CMakeLists.txt index 5bfba029162..83d1c751df4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1551,6 +1551,7 @@ target_link_libraries(end2end_tests endif() add_library(gpr + src/core/lib/event_engine/thread_local.cc src/core/lib/gpr/alloc.cc src/core/lib/gpr/atm.cc src/core/lib/gpr/cpu_iphone.cc diff --git a/Makefile b/Makefile index 339efe539f9..344731eec5f 100644 --- a/Makefile +++ b/Makefile @@ -840,6 +840,7 @@ endif # start of build recipe for library "gpr" (generated by makelib(lib) template function) LIBGPR_SRC = \ + src/core/lib/event_engine/thread_local.cc \ src/core/lib/gpr/alloc.cc \ src/core/lib/gpr/atm.cc \ src/core/lib/gpr/cpu_iphone.cc \ diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 72889dba85c..b2f6f1a78b8 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -170,6 +170,7 @@ libs: - include/grpc/support/thd_id.h - include/grpc/support/time.h headers: + - src/core/lib/event_engine/thread_local.h - src/core/lib/gpr/alloc.h - src/core/lib/gpr/string.h - src/core/lib/gpr/time_precise.h @@ -196,6 +197,7 @@ libs: - src/core/lib/gprpp/thd.h - src/core/lib/gprpp/time_util.h src: + - src/core/lib/event_engine/thread_local.cc - src/core/lib/gpr/alloc.cc - src/core/lib/gpr/atm.cc - src/core/lib/gpr/cpu_iphone.cc diff --git a/config.m4 b/config.m4 index 8f53ee8372d..98b27744058 100644 --- a/config.m4 +++ b/config.m4 @@ -532,6 +532,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/event_engine/slice.cc \ src/core/lib/event_engine/slice_buffer.cc \ src/core/lib/event_engine/tcp_socket_utils.cc \ + src/core/lib/event_engine/thread_local.cc \ src/core/lib/event_engine/thread_pool.cc \ src/core/lib/event_engine/time_util.cc \ src/core/lib/event_engine/trace.cc \ diff --git a/config.w32 b/config.w32 index 806f4ce16ab..12bf9850cc3 100644 --- a/config.w32 +++ b/config.w32 @@ -498,6 +498,7 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\event_engine\\slice.cc " + "src\\core\\lib\\event_engine\\slice_buffer.cc " + "src\\core\\lib\\event_engine\\tcp_socket_utils.cc " + + "src\\core\\lib\\event_engine\\thread_local.cc " + "src\\core\\lib\\event_engine\\thread_pool.cc " + "src\\core\\lib\\event_engine\\time_util.cc " + "src\\core\\lib\\event_engine\\trace.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 14c37a71674..edbf7f1f6a4 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -766,6 +766,7 @@ Pod::Spec.new do |s| 'src/core/lib/event_engine/shim.h', 'src/core/lib/event_engine/socket_notifier.h', 'src/core/lib/event_engine/tcp_socket_utils.h', + 'src/core/lib/event_engine/thread_local.h', 'src/core/lib/event_engine/thread_pool.h', 'src/core/lib/event_engine/time_util.h', 'src/core/lib/event_engine/trace.h', @@ -1698,6 +1699,7 @@ Pod::Spec.new do |s| 'src/core/lib/event_engine/shim.h', 'src/core/lib/event_engine/socket_notifier.h', 'src/core/lib/event_engine/tcp_socket_utils.h', + 'src/core/lib/event_engine/thread_local.h', 'src/core/lib/event_engine/thread_pool.h', 'src/core/lib/event_engine/time_util.h', 'src/core/lib/event_engine/trace.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 40f483ec64e..50c23979156 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -1187,6 +1187,8 @@ Pod::Spec.new do |s| 'src/core/lib/event_engine/socket_notifier.h', 'src/core/lib/event_engine/tcp_socket_utils.cc', 'src/core/lib/event_engine/tcp_socket_utils.h', + 'src/core/lib/event_engine/thread_local.cc', + 'src/core/lib/event_engine/thread_local.h', 'src/core/lib/event_engine/thread_pool.cc', 'src/core/lib/event_engine/thread_pool.h', 'src/core/lib/event_engine/time_util.cc', @@ -2382,6 +2384,7 @@ Pod::Spec.new do |s| 'src/core/lib/event_engine/shim.h', 'src/core/lib/event_engine/socket_notifier.h', 'src/core/lib/event_engine/tcp_socket_utils.h', + 'src/core/lib/event_engine/thread_local.h', 'src/core/lib/event_engine/thread_pool.h', 'src/core/lib/event_engine/time_util.h', 'src/core/lib/event_engine/trace.h', diff --git a/grpc.gemspec b/grpc.gemspec index 2165201a6d9..12940725429 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1096,6 +1096,8 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/event_engine/socket_notifier.h ) s.files += %w( src/core/lib/event_engine/tcp_socket_utils.cc ) s.files += %w( src/core/lib/event_engine/tcp_socket_utils.h ) + s.files += %w( src/core/lib/event_engine/thread_local.cc ) + s.files += %w( src/core/lib/event_engine/thread_local.h ) s.files += %w( src/core/lib/event_engine/thread_pool.cc ) s.files += %w( src/core/lib/event_engine/thread_pool.h ) s.files += %w( src/core/lib/event_engine/time_util.cc ) diff --git a/grpc.gyp b/grpc.gyp index 0901d343a10..a9ee344878a 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -305,6 +305,7 @@ 'absl/types:variant', ], 'sources': [ + 'src/core/lib/event_engine/thread_local.cc', 'src/core/lib/gpr/alloc.cc', 'src/core/lib/gpr/atm.cc', 'src/core/lib/gpr/cpu_iphone.cc', diff --git a/package.xml b/package.xml index b709ab324ac..c8806f5cd56 100644 --- a/package.xml +++ b/package.xml @@ -1078,6 +1078,8 @@ + + diff --git a/src/core/BUILD b/src/core/BUILD index 243ac94d150..cb640fa0afc 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -1471,6 +1471,13 @@ grpc_cc_library( ], ) +grpc_cc_library( + name = "event_engine_thread_local", + srcs = ["lib/event_engine/thread_local.cc"], + hdrs = ["lib/event_engine/thread_local.h"], + deps = ["//:gpr_platform"], +) + grpc_cc_library( name = "event_engine_thread_pool", srcs = ["lib/event_engine/thread_pool.cc"], @@ -1484,6 +1491,7 @@ grpc_cc_library( ], deps = [ "event_engine_executor", + "event_engine_thread_local", "forkable", "time", "useful", diff --git a/src/core/lib/event_engine/thread_local.cc b/src/core/lib/event_engine/thread_local.cc new file mode 100644 index 00000000000..1dce8d83bcd --- /dev/null +++ b/src/core/lib/event_engine/thread_local.cc @@ -0,0 +1,29 @@ +// Copyright 2023 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +#include + +#include "src/core/lib/event_engine/thread_local.h" + +namespace grpc_event_engine { +namespace experimental { + +namespace { +thread_local bool g_thread_local{false}; +} // namespace + +void ThreadLocal::SetIsEventEngineThread(bool is) { g_thread_local = is; } +bool ThreadLocal::IsEventEngineThread() { return g_thread_local; } + +} // namespace experimental +} // namespace grpc_event_engine diff --git a/src/core/lib/event_engine/thread_local.h b/src/core/lib/event_engine/thread_local.h new file mode 100644 index 00000000000..986df908aca --- /dev/null +++ b/src/core/lib/event_engine/thread_local.h @@ -0,0 +1,32 @@ +// Copyright 2023 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +#ifndef GRPC_SRC_CORE_LIB_EVENT_ENGINE_THREAD_LOCAL_H +#define GRPC_SRC_CORE_LIB_EVENT_ENGINE_THREAD_LOCAL_H +#include + +namespace grpc_event_engine { +namespace experimental { + +/// A lightweight facility to allow gpr's fork handlers and +/// EventEngine::Forkables to coordinate. +class ThreadLocal { + public: + static void SetIsEventEngineThread(bool is_local); + static bool IsEventEngineThread(); +}; + +} // namespace experimental +} // namespace grpc_event_engine + +#endif // GRPC_SRC_CORE_LIB_EVENT_ENGINE_THREAD_LOCAL_H \ No newline at end of file diff --git a/src/core/lib/event_engine/thread_pool.cc b/src/core/lib/event_engine/thread_pool.cc index 35d8c651f81..60abfe7004c 100644 --- a/src/core/lib/event_engine/thread_pool.cc +++ b/src/core/lib/event_engine/thread_pool.cc @@ -30,18 +30,13 @@ #include +#include "src/core/lib/event_engine/thread_local.h" #include "src/core/lib/gprpp/thd.h" #include "src/core/lib/gprpp/time.h" namespace grpc_event_engine { namespace experimental { -namespace { -// TODO(drfloob): Remove this, and replace it with the WorkQueue* for the -// current thread (with nullptr indicating not a threadpool thread). -thread_local bool g_threadpool_thread; -} // namespace - void ThreadPool::StartThread(StatePtr state, StartThreadReason reason) { state->thread_count.Add(); const auto now = grpc_core::Timestamp::Now(); @@ -76,7 +71,7 @@ void ThreadPool::StartThread(StatePtr state, StartThreadReason reason) { "event_engine", [](void* arg) { std::unique_ptr a(static_cast(arg)); - g_threadpool_thread = true; + ThreadLocal::SetIsEventEngineThread(true); switch (a->reason) { case StartThreadReason::kInitialPool: break; @@ -148,14 +143,18 @@ ThreadPool::ThreadPool() { } } +bool ThreadPool::IsThreadPoolThread() { + return ThreadLocal::IsEventEngineThread(); +} + void ThreadPool::Quiesce() { state_->queue.SetShutdown(); // Wait until all threads are exited. // Note that if this is a threadpool thread then we won't exit this thread // until the callstack unwinds a little, so we need to wait for just one // thread running instead of zero. - state_->thread_count.BlockUntilThreadCount(g_threadpool_thread ? 1 : 0, - "shutting down"); + state_->thread_count.BlockUntilThreadCount( + ThreadLocal::IsEventEngineThread() ? 1 : 0, "shutting down"); quiesced_.store(true, std::memory_order_relaxed); } diff --git a/src/core/lib/event_engine/thread_pool.h b/src/core/lib/event_engine/thread_pool.h index 63113e999f6..39c7a78e8ce 100644 --- a/src/core/lib/event_engine/thread_pool.h +++ b/src/core/lib/event_engine/thread_pool.h @@ -59,6 +59,9 @@ class ThreadPool final : public Forkable, public Executor { void PostforkParent() override; void PostforkChild() override; + // Returns true if the current thread is a thread pool thread. + static bool IsThreadPoolThread(); + private: class Queue { public: diff --git a/src/core/lib/gprpp/fork.cc b/src/core/lib/gprpp/fork.cc index 55657ed69d6..127ca8a4455 100644 --- a/src/core/lib/gprpp/fork.cc +++ b/src/core/lib/gprpp/fork.cc @@ -24,6 +24,7 @@ #include #include +#include "src/core/lib/event_engine/thread_local.h" #include "src/core/lib/gprpp/global_config_env.h" #include "src/core/lib/gprpp/no_destruct.h" @@ -63,6 +64,12 @@ class ExecCtxState { } void IncExecCtxCount() { + // EventEngine is expected to terminate all threads before fork, and so this + // extra work is unnecessary + if (grpc_event_engine::experimental::ThreadLocal::IsEventEngineThread()) { + gpr_atm_no_barrier_fetch_add(&count_, 1); + return; + } gpr_atm count = gpr_atm_no_barrier_load(&count_); while (true) { if (count <= BLOCKED(1)) { diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index ca3cccdae35..6f474ac2029 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -507,6 +507,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/event_engine/slice.cc', 'src/core/lib/event_engine/slice_buffer.cc', 'src/core/lib/event_engine/tcp_socket_utils.cc', + 'src/core/lib/event_engine/thread_local.cc', 'src/core/lib/event_engine/thread_pool.cc', 'src/core/lib/event_engine/time_util.cc', 'src/core/lib/event_engine/trace.cc', diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index ca2bba26e35..0ce7d524120 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -2091,6 +2091,8 @@ src/core/lib/event_engine/slice_buffer.cc \ src/core/lib/event_engine/socket_notifier.h \ src/core/lib/event_engine/tcp_socket_utils.cc \ src/core/lib/event_engine/tcp_socket_utils.h \ +src/core/lib/event_engine/thread_local.cc \ +src/core/lib/event_engine/thread_local.h \ src/core/lib/event_engine/thread_pool.cc \ src/core/lib/event_engine/thread_pool.h \ src/core/lib/event_engine/time_util.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 29a23fbefcd..047984aef50 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1869,6 +1869,8 @@ src/core/lib/event_engine/slice_buffer.cc \ src/core/lib/event_engine/socket_notifier.h \ src/core/lib/event_engine/tcp_socket_utils.cc \ src/core/lib/event_engine/tcp_socket_utils.h \ +src/core/lib/event_engine/thread_local.cc \ +src/core/lib/event_engine/thread_local.h \ src/core/lib/event_engine/thread_pool.cc \ src/core/lib/event_engine/thread_pool.h \ src/core/lib/event_engine/time_util.cc \