From 888a10d8a6a30ec0acd7c3cbe9656eb2aa6d237f Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 22 Nov 2021 13:43:31 -0800 Subject: [PATCH] Reland arena based promises (#28174) * Revert "Revert "Arena allocated promise (#28023)" (#28171)" This reverts commit 9b07a81b1a9ed10ffae65412330d4fe27ba98519. * fix * Automated change: Fix sanity tests Co-authored-by: ctiller --- BUILD | 15 ++ CMakeLists.txt | 103 +++++++++++++ build_autogenerated.yaml | 119 +++++++++++++++ src/core/lib/promise/arena_promise.h | 184 ++++++++++++++++++++++++ test/core/promise/BUILD | 12 ++ test/core/promise/arena_promise_test.cc | 45 ++++++ tools/run_tests/generated/tests.json | 24 ++++ 7 files changed, 502 insertions(+) create mode 100644 src/core/lib/promise/arena_promise.h create mode 100644 test/core/promise/arena_promise_test.cc diff --git a/BUILD b/BUILD index 1fbe1d44c47..b4ce0fb9692 100644 --- a/BUILD +++ b/BUILD @@ -1087,6 +1087,21 @@ grpc_cc_library( ], ) +grpc_cc_library( + name = "arena_promise", + external_deps = [ + "absl/types:optional", + ], + language = "c++", + public_hdrs = [ + "src/core/lib/promise/arena_promise.h", + ], + deps = [ + "gpr_base", + "poll", + ], +) + grpc_cc_library( name = "promise_like", language = "c++", diff --git a/CMakeLists.txt b/CMakeLists.txt index 035af441756..11164d6e737 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -743,6 +743,7 @@ if(gRPC_BUILD_TESTS) add_dependencies(buildtests_cxx alts_concurrent_connectivity_test) endif() add_dependencies(buildtests_cxx alts_util_test) + add_dependencies(buildtests_cxx arena_promise_test) add_dependencies(buildtests_cxx async_end2end_test) add_dependencies(buildtests_cxx auth_property_iterator_test) add_dependencies(buildtests_cxx authorization_matchers_test) @@ -7854,6 +7855,108 @@ target_link_libraries(alts_util_test ) +endif() +if(gRPC_BUILD_TESTS) + +add_executable(arena_promise_test + src/core/ext/upb-generated/google/api/annotations.upb.c + src/core/ext/upb-generated/google/api/expr/v1alpha1/checked.upb.c + src/core/ext/upb-generated/google/api/expr/v1alpha1/syntax.upb.c + src/core/ext/upb-generated/google/api/http.upb.c + src/core/ext/upb-generated/google/protobuf/any.upb.c + src/core/ext/upb-generated/google/protobuf/duration.upb.c + src/core/ext/upb-generated/google/protobuf/empty.upb.c + src/core/ext/upb-generated/google/protobuf/struct.upb.c + src/core/ext/upb-generated/google/protobuf/timestamp.upb.c + src/core/ext/upb-generated/google/protobuf/wrappers.upb.c + src/core/ext/upb-generated/google/rpc/status.upb.c + src/core/lib/gpr/alloc.cc + src/core/lib/gpr/atm.cc + src/core/lib/gpr/cpu_iphone.cc + src/core/lib/gpr/cpu_linux.cc + src/core/lib/gpr/cpu_posix.cc + src/core/lib/gpr/cpu_windows.cc + src/core/lib/gpr/env_linux.cc + src/core/lib/gpr/env_posix.cc + src/core/lib/gpr/env_windows.cc + src/core/lib/gpr/log.cc + src/core/lib/gpr/log_android.cc + src/core/lib/gpr/log_linux.cc + src/core/lib/gpr/log_posix.cc + src/core/lib/gpr/log_windows.cc + src/core/lib/gpr/murmur_hash.cc + src/core/lib/gpr/string.cc + src/core/lib/gpr/string_posix.cc + src/core/lib/gpr/string_util_windows.cc + src/core/lib/gpr/string_windows.cc + src/core/lib/gpr/sync.cc + src/core/lib/gpr/sync_abseil.cc + src/core/lib/gpr/sync_posix.cc + src/core/lib/gpr/sync_windows.cc + src/core/lib/gpr/time.cc + src/core/lib/gpr/time_posix.cc + src/core/lib/gpr/time_precise.cc + src/core/lib/gpr/time_windows.cc + src/core/lib/gpr/tmpfile_msys.cc + src/core/lib/gpr/tmpfile_posix.cc + src/core/lib/gpr/tmpfile_windows.cc + src/core/lib/gpr/wrap_memcpy.cc + src/core/lib/gprpp/arena.cc + src/core/lib/gprpp/examine_stack.cc + src/core/lib/gprpp/fork.cc + src/core/lib/gprpp/global_config_env.cc + src/core/lib/gprpp/host_port.cc + src/core/lib/gprpp/mpscq.cc + src/core/lib/gprpp/stat_posix.cc + src/core/lib/gprpp/stat_windows.cc + src/core/lib/gprpp/status_helper.cc + src/core/lib/gprpp/thd_posix.cc + src/core/lib/gprpp/thd_windows.cc + src/core/lib/gprpp/time_util.cc + src/core/lib/profiling/basic_timers.cc + src/core/lib/profiling/stap_timers.cc + test/core/promise/arena_promise_test.cc + third_party/googletest/googletest/src/gtest-all.cc + third_party/googletest/googlemock/src/gmock-all.cc +) + +target_include_directories(arena_promise_test + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + ${_gRPC_RE2_INCLUDE_DIR} + ${_gRPC_SSL_INCLUDE_DIR} + ${_gRPC_UPB_GENERATED_DIR} + ${_gRPC_UPB_GRPC_GENERATED_DIR} + ${_gRPC_UPB_INCLUDE_DIR} + ${_gRPC_XXHASH_INCLUDE_DIR} + ${_gRPC_ZLIB_INCLUDE_DIR} + third_party/googletest/googletest/include + third_party/googletest/googletest + third_party/googletest/googlemock/include + third_party/googletest/googlemock + ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(arena_promise_test + ${_gRPC_PROTOBUF_LIBRARIES} + ${_gRPC_ALLTARGETS_LIBRARIES} + absl::base + absl::core_headers + absl::memory + absl::status + absl::cord + absl::str_format + absl::strings + absl::synchronization + absl::time + absl::optional + absl::variant + upb +) + + endif() if(gRPC_BUILD_TESTS) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 9ff9e89b6cc..93210d9c393 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -4535,6 +4535,125 @@ targets: deps: - grpc++_alts - grpc++_test_util +- name: arena_promise_test + gtest: true + build: test + language: c++ + headers: + - src/core/ext/upb-generated/google/api/annotations.upb.h + - src/core/ext/upb-generated/google/api/expr/v1alpha1/checked.upb.h + - src/core/ext/upb-generated/google/api/expr/v1alpha1/syntax.upb.h + - src/core/ext/upb-generated/google/api/http.upb.h + - src/core/ext/upb-generated/google/protobuf/any.upb.h + - src/core/ext/upb-generated/google/protobuf/duration.upb.h + - src/core/ext/upb-generated/google/protobuf/empty.upb.h + - src/core/ext/upb-generated/google/protobuf/struct.upb.h + - src/core/ext/upb-generated/google/protobuf/timestamp.upb.h + - src/core/ext/upb-generated/google/protobuf/wrappers.upb.h + - src/core/ext/upb-generated/google/rpc/status.upb.h + - src/core/lib/gpr/alloc.h + - src/core/lib/gpr/env.h + - src/core/lib/gpr/murmur_hash.h + - src/core/lib/gpr/spinlock.h + - src/core/lib/gpr/string.h + - src/core/lib/gpr/string_windows.h + - src/core/lib/gpr/time_precise.h + - src/core/lib/gpr/tls.h + - src/core/lib/gpr/tmpfile.h + - src/core/lib/gpr/useful.h + - src/core/lib/gprpp/arena.h + - src/core/lib/gprpp/construct_destruct.h + - src/core/lib/gprpp/debug_location.h + - src/core/lib/gprpp/examine_stack.h + - src/core/lib/gprpp/fork.h + - src/core/lib/gprpp/global_config.h + - src/core/lib/gprpp/global_config_custom.h + - src/core/lib/gprpp/global_config_env.h + - src/core/lib/gprpp/global_config_generic.h + - src/core/lib/gprpp/host_port.h + - src/core/lib/gprpp/manual_constructor.h + - src/core/lib/gprpp/memory.h + - src/core/lib/gprpp/mpscq.h + - src/core/lib/gprpp/stat.h + - src/core/lib/gprpp/status_helper.h + - src/core/lib/gprpp/sync.h + - src/core/lib/gprpp/thd.h + - src/core/lib/gprpp/time_util.h + - src/core/lib/profiling/timers.h + - src/core/lib/promise/arena_promise.h + - src/core/lib/promise/poll.h + src: + - src/core/ext/upb-generated/google/api/annotations.upb.c + - src/core/ext/upb-generated/google/api/expr/v1alpha1/checked.upb.c + - src/core/ext/upb-generated/google/api/expr/v1alpha1/syntax.upb.c + - src/core/ext/upb-generated/google/api/http.upb.c + - src/core/ext/upb-generated/google/protobuf/any.upb.c + - src/core/ext/upb-generated/google/protobuf/duration.upb.c + - src/core/ext/upb-generated/google/protobuf/empty.upb.c + - src/core/ext/upb-generated/google/protobuf/struct.upb.c + - src/core/ext/upb-generated/google/protobuf/timestamp.upb.c + - src/core/ext/upb-generated/google/protobuf/wrappers.upb.c + - src/core/ext/upb-generated/google/rpc/status.upb.c + - src/core/lib/gpr/alloc.cc + - src/core/lib/gpr/atm.cc + - src/core/lib/gpr/cpu_iphone.cc + - src/core/lib/gpr/cpu_linux.cc + - src/core/lib/gpr/cpu_posix.cc + - src/core/lib/gpr/cpu_windows.cc + - src/core/lib/gpr/env_linux.cc + - src/core/lib/gpr/env_posix.cc + - src/core/lib/gpr/env_windows.cc + - src/core/lib/gpr/log.cc + - src/core/lib/gpr/log_android.cc + - src/core/lib/gpr/log_linux.cc + - src/core/lib/gpr/log_posix.cc + - src/core/lib/gpr/log_windows.cc + - src/core/lib/gpr/murmur_hash.cc + - src/core/lib/gpr/string.cc + - src/core/lib/gpr/string_posix.cc + - src/core/lib/gpr/string_util_windows.cc + - src/core/lib/gpr/string_windows.cc + - src/core/lib/gpr/sync.cc + - src/core/lib/gpr/sync_abseil.cc + - src/core/lib/gpr/sync_posix.cc + - src/core/lib/gpr/sync_windows.cc + - src/core/lib/gpr/time.cc + - src/core/lib/gpr/time_posix.cc + - src/core/lib/gpr/time_precise.cc + - src/core/lib/gpr/time_windows.cc + - src/core/lib/gpr/tmpfile_msys.cc + - src/core/lib/gpr/tmpfile_posix.cc + - src/core/lib/gpr/tmpfile_windows.cc + - src/core/lib/gpr/wrap_memcpy.cc + - src/core/lib/gprpp/arena.cc + - src/core/lib/gprpp/examine_stack.cc + - src/core/lib/gprpp/fork.cc + - src/core/lib/gprpp/global_config_env.cc + - src/core/lib/gprpp/host_port.cc + - src/core/lib/gprpp/mpscq.cc + - src/core/lib/gprpp/stat_posix.cc + - src/core/lib/gprpp/stat_windows.cc + - src/core/lib/gprpp/status_helper.cc + - src/core/lib/gprpp/thd_posix.cc + - src/core/lib/gprpp/thd_windows.cc + - src/core/lib/gprpp/time_util.cc + - src/core/lib/profiling/basic_timers.cc + - src/core/lib/profiling/stap_timers.cc + - test/core/promise/arena_promise_test.cc + deps: + - absl/base:base + - absl/base:core_headers + - absl/memory:memory + - absl/status:status + - absl/strings:cord + - absl/strings:str_format + - absl/strings:strings + - absl/synchronization:synchronization + - absl/time:time + - absl/types:optional + - absl/types:variant + - upb + uses_polling: false - name: async_end2end_test gtest: true build: test diff --git a/src/core/lib/promise/arena_promise.h b/src/core/lib/promise/arena_promise.h new file mode 100644 index 00000000000..35d914d3c2b --- /dev/null +++ b/src/core/lib/promise/arena_promise.h @@ -0,0 +1,184 @@ +// Copyright 2021 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_CORE_LIB_PROMISE_ARENA_PROMISE_H +#define GRPC_CORE_LIB_PROMISE_ARENA_PROMISE_H + +#include + +#include + +#include "src/core/lib/gprpp/arena.h" +#include "src/core/lib/promise/poll.h" + +namespace grpc_core { + +namespace arena_promise_detail { + +// Type erased promise stored in the arena. +template +class ImplInterface { + public: + // Poll the promise, once. + virtual Poll PollOnce() = 0; + // Destroy the underlying callable object if there is one. + // Since we don't delete (the arena owns the memory) but we may need to call a + // destructor, we expose this for when the ArenaPromise object is destroyed. + virtual void Destroy() = 0; + + protected: + ~ImplInterface() = default; +}; + +// Implementation of ImplInterface for an empty object. +// Used when an empty ArenaPromise is created, or when the ArenaPromise is moved +// from. Since in either case these objects should not be polled, we simply +// crash if it is. +template +class NullImpl final : public ImplInterface { + public: + Poll PollOnce() override { + abort(); + GPR_UNREACHABLE_CODE(return Pending{}); + } + void Destroy() override {} + + static ImplInterface* Get() { + static NullImpl instance; + return &instance; + } + + private: + ~NullImpl() = default; +}; + +// Implementation of ImplInterface for a callable object. +template +class CallableImpl final : public ImplInterface { + public: + explicit CallableImpl(Callable&& callable) : callable_(std::move(callable)) {} + // Forward polls to the callable object. + Poll PollOnce() override { return callable_(); } + // Destroy destructs the callable object. + void Destroy() override { this->~CallableImpl(); } + + private: + // Should only be called by Destroy(). + ~CallableImpl() = default; + + Callable callable_; +}; + +// If a callable object is empty we can substitute any instance of that callable +// for the one we call (for how could we tell the difference)? +// Since this corresponds to a lambda with no fields, and we expect these to be +// reasonably common, we can elide the arena allocation entirely and simply poll +// a global shared instance. +// (this comes up often when the promise only accesses context data from the +// containing activity). +template +class SharedImpl final : public ImplInterface, private Callable { + public: + // Call the callable, or at least an exact duplicate of it - if you have no + // members, all your instances look the same. + Poll PollOnce() override { return Callable::operator()(); } + // Nothing to destroy. + void Destroy() override {} + // Return a pointer to the shared instance - these are singletons, and are + // needed just to get the vtable in place. + static SharedImpl* Get(Callable&& callable) { + static_assert(sizeof(SharedImpl) == sizeof(void*), + "SharedImpl should be pointer sized"); + static SharedImpl impl(std::forward(callable)); + return &impl; + } + + private: + explicit SharedImpl(Callable&& callable) + : Callable(std::forward(callable)) {} + ~SharedImpl() = default; +}; + +// Redirector type: given a callable type, expose a Make() function that creates +// the appropriate underlying implementation. +template +struct ChooseImplForCallable; + +template +struct ChooseImplForCallable< + T, Callable, absl::enable_if_t::value>> { + static ImplInterface* Make(Arena* arena, Callable&& callable) { + return arena->template New>( + std::forward(callable)); + } +}; + +template +struct ChooseImplForCallable< + T, Callable, absl::enable_if_t::value>> { + static ImplInterface* Make(Arena*, Callable&& callable) { + return SharedImpl::Get(std::forward(callable)); + } +}; + +// Wrap ChooseImplForCallable with a friend approachable syntax. +template +ImplInterface* MakeImplForCallable(Arena* arena, Callable&& callable) { + return ChooseImplForCallable::Make( + arena, std::forward(callable)); +} + +} // namespace arena_promise_detail + +// A promise for which the state memory is allocated from an arena. +template +class ArenaPromise { + public: + // Construct an empty, uncallable, invalid ArenaPromise. + ArenaPromise() = default; + + // Construct an ArenaPromise that will call the given callable when polled. + template + ArenaPromise(Arena* arena, Callable&& callable) + : impl_(arena_promise_detail::MakeImplForCallable( + arena, std::forward(callable))) {} + + // ArenaPromise is not copyable. + ArenaPromise(const ArenaPromise&) = delete; + ArenaPromise& operator=(const ArenaPromise&) = delete; + // ArenaPromise is movable. + ArenaPromise(ArenaPromise&& other) noexcept : impl_(other.impl_) { + other.impl_ = arena_promise_detail::NullImpl::Get(); + } + ArenaPromise& operator=(ArenaPromise&& other) noexcept { + impl_ = other.impl_; + other.impl_ = arena_promise_detail::NullImpl::Get(); + return *this; + } + + // Destruction => call Destroy on the underlying impl object. + ~ArenaPromise() { impl_->Destroy(); } + + // Expose the promise interface: a call operator that returns Poll. + Poll operator()() { return impl_->PollOnce(); } + + private: + // Underlying impl object. + arena_promise_detail::ImplInterface* impl_ = + arena_promise_detail::NullImpl::Get(); +}; + +} // namespace grpc_core + +#endif /* GRPC_CORE_LIB_PROMISE_ARENA_PROMISE_H */ diff --git a/test/core/promise/BUILD b/test/core/promise/BUILD index 04fe2bafc20..66dd1d9ab5e 100644 --- a/test/core/promise/BUILD +++ b/test/core/promise/BUILD @@ -67,6 +67,18 @@ grpc_cc_test( ], ) +grpc_cc_test( + name = "arena_promise_test", + srcs = ["arena_promise_test.cc"], + external_deps = ["gtest"], + language = "c++", + uses_polling = False, + deps = [ + "//:arena_promise", + "//test/core/util:grpc_suppressions", + ], +) + grpc_cc_test( # Why promise_map_test and not map_test? # third_party/benchmark defines a map_test in its cmakefile, and we depend diff --git a/test/core/promise/arena_promise_test.cc b/test/core/promise/arena_promise_test.cc new file mode 100644 index 00000000000..561373202c7 --- /dev/null +++ b/test/core/promise/arena_promise_test.cc @@ -0,0 +1,45 @@ +// Copyright 2021 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 "src/core/lib/promise/arena_promise.h" + +#include + +#include + +namespace grpc_core { + +TEST(ArenaPromiseTest, AllocatedWorks) { + auto arena = MakeScopedArena(1024); + int x = 42; + ArenaPromise p(arena.get(), [x] { return Poll(x); }); + EXPECT_EQ(p(), Poll(42)); + p = ArenaPromise(arena.get(), [] { return Poll(43); }); + EXPECT_EQ(p(), Poll(43)); +} + +TEST(ArenaPromiseTest, DestructionWorks) { + auto arena = MakeScopedArena(1024); + auto x = std::make_shared(42); + auto p = ArenaPromise(arena.get(), [x] { return Poll(*x); }); + ArenaPromise q(std::move(p)); + EXPECT_EQ(q(), Poll(42)); +} + +} // namespace grpc_core + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index d0ac6b7d89f..1962d17f633 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -3045,6 +3045,30 @@ ], "uses_polling": true }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "arena_promise_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": false + }, { "args": [], "benchmark": false,