From 54651a71682c52917acab1110fe55bc6a07bd0f3 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 10 Aug 2023 09:40:13 -0700 Subject: [PATCH] [promises] New `Seq`, `TrySeq` implementation (#33991) Our current implementation of `Seq`, `TrySeq` leverage some complicated template stuff to work, which makes them hard to maintain. I've been thinking about ways to simplify that for some time and had something like this in mind - using a code generator that's at least a little more understandable to code generate most of the complexity into a file that is checkable. Concurrently - I have a cool optimization in mind - but it requires that we can move promises after polling, which is a contract change. I'm going to work through the set of primitives we have in the coming weeks and change that contract to enable the optimization. --------- Co-authored-by: ctiller --- BUILD | 2 - CMakeLists.txt | 6 - Package.swift | 2 +- build_autogenerated.yaml | 34 +- gRPC-C++.podspec | 4 +- gRPC-Core.podspec | 4 +- grpc.gemspec | 2 +- package.xml | 2 +- src/core/BUILD | 24 +- .../channel_idle/channel_idle_filter.cc | 1 + src/core/lib/channel/connected_channel.cc | 1 - src/core/lib/channel/promise_based_filter.cc | 2 +- src/core/lib/promise/detail/basic_seq.h | 373 +-- src/core/lib/promise/detail/seq_state.h | 2076 +++++++++++++++++ src/core/lib/promise/seq.h | 21 +- src/core/lib/promise/try_seq.h | 36 +- src/core/lib/resource_quota/memory_quota.cc | 1 - .../security/transport/client_auth_filter.cc | 2 +- .../security/transport/server_auth_filter.cc | 2 + src/core/lib/surface/call.cc | 1 - src/core/lib/surface/server.cc | 2 +- test/core/filters/filter_test.cc | 2 +- test/core/promise/BUILD | 3 - test/core/promise/for_each_test.cc | 1 - test/core/promise/loop_test.cc | 3 +- test/core/promise/map_pipe_test.cc | 1 - test/core/promise/promise_fuzzer.cc | 2 + test/core/promise/seq_test.cc | 2 + test/core/promise/try_seq_metadata_test.cc | 2 + test/core/promise/try_seq_test.cc | 1 + tools/codegen/core/gen_seq.py | 271 +++ tools/doxygen/Doxyfile.c++.internal | 2 +- tools/doxygen/Doxyfile.core.internal | 2 +- 33 files changed, 2457 insertions(+), 433 deletions(-) create mode 100644 src/core/lib/promise/detail/seq_state.h create mode 100755 tools/codegen/core/gen_seq.py diff --git a/BUILD b/BUILD index a1a9c395447..c7a7e844c43 100644 --- a/BUILD +++ b/BUILD @@ -1534,7 +1534,6 @@ grpc_cc_library( "//src/core:arena", "//src/core:arena_promise", "//src/core:atomic_utils", - "//src/core:basic_seq", "//src/core:bitset", "//src/core:cancel_callback", "//src/core:channel_args", @@ -1799,7 +1798,6 @@ grpc_cc_library( "//src/core:activity", "//src/core:arena", "//src/core:arena_promise", - "//src/core:basic_seq", "//src/core:channel_args", "//src/core:channel_fwd", "//src/core:closure", diff --git a/CMakeLists.txt b/CMakeLists.txt index 5b5e57d76ab..3e281c5c561 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8777,7 +8777,6 @@ target_link_libraries(chunked_vector_test absl::hash absl::type_traits absl::statusor - absl::utility gpr upb ) @@ -12110,7 +12109,6 @@ target_link_libraries(flow_control_test absl::hash absl::type_traits absl::statusor - absl::utility gpr upb ) @@ -15292,7 +15290,6 @@ target_link_libraries(interceptor_list_test absl::hash absl::type_traits absl::statusor - absl::utility gpr upb ) @@ -16227,7 +16224,6 @@ target_link_libraries(loop_test ${_gRPC_ALLTARGETS_LIBRARIES} absl::type_traits absl::statusor - absl::utility gpr ) @@ -22151,7 +22147,6 @@ target_link_libraries(seq_test ${_gRPC_ZLIB_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} absl::type_traits - absl::utility gpr ) @@ -26523,7 +26518,6 @@ target_link_libraries(try_seq_test ${_gRPC_ALLTARGETS_LIBRARIES} absl::type_traits absl::statusor - absl::utility gpr ) diff --git a/Package.swift b/Package.swift index 56699d86e08..afeb98b7f1c 100644 --- a/Package.swift +++ b/Package.swift @@ -1447,8 +1447,8 @@ let package = Package( "src/core/lib/promise/detail/basic_seq.h", "src/core/lib/promise/detail/promise_factory.h", "src/core/lib/promise/detail/promise_like.h", + "src/core/lib/promise/detail/seq_state.h", "src/core/lib/promise/detail/status.h", - "src/core/lib/promise/detail/switch.h", "src/core/lib/promise/exec_ctx_wakeup_scheduler.h", "src/core/lib/promise/for_each.h", "src/core/lib/promise/if.h", diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index a21e36895b1..e780ca50f6d 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -857,8 +857,8 @@ libs: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/for_each.h - src/core/lib/promise/if.h @@ -2253,8 +2253,8 @@ libs: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/for_each.h - src/core/lib/promise/if.h @@ -3754,8 +3754,8 @@ libs: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/for_each.h - src/core/lib/promise/if.h @@ -4307,8 +4307,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/join.h - src/core/lib/promise/poll.h - src/core/lib/promise/promise.h @@ -6115,8 +6115,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/loop.h - src/core/lib/promise/map.h @@ -6167,7 +6167,6 @@ targets: - absl/hash:hash - absl/meta:type_traits - absl/status:statusor - - absl/utility:utility - gpr - upb uses_polling: false @@ -7761,8 +7760,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/loop.h - src/core/lib/promise/map.h @@ -7818,7 +7817,6 @@ targets: - absl/hash:hash - absl/meta:type_traits - absl/status:statusor - - absl/utility:utility - gpr - upb uses_polling: false @@ -7854,8 +7852,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/for_each.h - src/core/lib/promise/if.h @@ -8226,8 +8224,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/for_each.h - src/core/lib/promise/if.h @@ -9702,8 +9700,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/interceptor_list.h - src/core/lib/promise/loop.h @@ -9758,7 +9756,6 @@ targets: - absl/hash:hash - absl/meta:type_traits - absl/status:statusor - - absl/utility:utility - gpr - upb uses_polling: false @@ -10075,8 +10072,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/join.h - src/core/lib/promise/latch.h - src/core/lib/promise/poll.h @@ -10179,7 +10176,7 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h - - src/core/lib/promise/detail/switch.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/loop.h - src/core/lib/promise/poll.h - src/core/lib/promise/seq.h @@ -10188,7 +10185,6 @@ targets: deps: - absl/meta:type_traits - absl/status:statusor - - absl/utility:utility - gpr uses_polling: false - name: map_pipe_test @@ -10223,8 +10219,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/exec_ctx_wakeup_scheduler.h - src/core/lib/promise/for_each.h - src/core/lib/promise/if.h @@ -13441,14 +13437,13 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h - - src/core/lib/promise/detail/switch.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/poll.h - src/core/lib/promise/seq.h src: - test/core/promise/seq_test.cc deps: - absl/meta:type_traits - - absl/utility:utility - gpr uses_polling: false - name: sequential_connectivity_test @@ -15414,8 +15409,8 @@ targets: - src/core/lib/promise/detail/basic_seq.h - src/core/lib/promise/detail/promise_factory.h - src/core/lib/promise/detail/promise_like.h + - src/core/lib/promise/detail/seq_state.h - src/core/lib/promise/detail/status.h - - src/core/lib/promise/detail/switch.h - src/core/lib/promise/poll.h - src/core/lib/promise/try_seq.h src: @@ -15423,7 +15418,6 @@ targets: deps: - absl/meta:type_traits - absl/status:statusor - - absl/utility:utility - gpr uses_polling: false - name: unique_type_name_test diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index d9590738516..a9ad7359874 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -951,8 +951,8 @@ Pod::Spec.new do |s| 'src/core/lib/promise/detail/basic_seq.h', 'src/core/lib/promise/detail/promise_factory.h', 'src/core/lib/promise/detail/promise_like.h', + 'src/core/lib/promise/detail/seq_state.h', 'src/core/lib/promise/detail/status.h', - 'src/core/lib/promise/detail/switch.h', 'src/core/lib/promise/exec_ctx_wakeup_scheduler.h', 'src/core/lib/promise/for_each.h', 'src/core/lib/promise/if.h', @@ -2001,8 +2001,8 @@ Pod::Spec.new do |s| 'src/core/lib/promise/detail/basic_seq.h', 'src/core/lib/promise/detail/promise_factory.h', 'src/core/lib/promise/detail/promise_like.h', + 'src/core/lib/promise/detail/seq_state.h', 'src/core/lib/promise/detail/status.h', - 'src/core/lib/promise/detail/switch.h', 'src/core/lib/promise/exec_ctx_wakeup_scheduler.h', 'src/core/lib/promise/for_each.h', 'src/core/lib/promise/if.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index c6f4811efe8..8a59d4bd0ae 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -1548,8 +1548,8 @@ Pod::Spec.new do |s| 'src/core/lib/promise/detail/basic_seq.h', 'src/core/lib/promise/detail/promise_factory.h', 'src/core/lib/promise/detail/promise_like.h', + 'src/core/lib/promise/detail/seq_state.h', 'src/core/lib/promise/detail/status.h', - 'src/core/lib/promise/detail/switch.h', 'src/core/lib/promise/exec_ctx_wakeup_scheduler.h', 'src/core/lib/promise/for_each.h', 'src/core/lib/promise/if.h', @@ -2737,8 +2737,8 @@ Pod::Spec.new do |s| 'src/core/lib/promise/detail/basic_seq.h', 'src/core/lib/promise/detail/promise_factory.h', 'src/core/lib/promise/detail/promise_like.h', + 'src/core/lib/promise/detail/seq_state.h', 'src/core/lib/promise/detail/status.h', - 'src/core/lib/promise/detail/switch.h', 'src/core/lib/promise/exec_ctx_wakeup_scheduler.h', 'src/core/lib/promise/for_each.h', 'src/core/lib/promise/if.h', diff --git a/grpc.gemspec b/grpc.gemspec index d3d4df3b6cc..c14953000cb 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1453,8 +1453,8 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/promise/detail/basic_seq.h ) s.files += %w( src/core/lib/promise/detail/promise_factory.h ) s.files += %w( src/core/lib/promise/detail/promise_like.h ) + s.files += %w( src/core/lib/promise/detail/seq_state.h ) s.files += %w( src/core/lib/promise/detail/status.h ) - s.files += %w( src/core/lib/promise/detail/switch.h ) s.files += %w( src/core/lib/promise/exec_ctx_wakeup_scheduler.h ) s.files += %w( src/core/lib/promise/for_each.h ) s.files += %w( src/core/lib/promise/if.h ) diff --git a/package.xml b/package.xml index 3a8afe9b3fd..e0cd73dc383 100644 --- a/package.xml +++ b/package.xml @@ -1435,8 +1435,8 @@ + - diff --git a/src/core/BUILD b/src/core/BUILD index cf94603020d..b6c7590a4fd 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -665,21 +665,30 @@ grpc_cc_library( grpc_cc_library( name = "basic_seq", - external_deps = [ - "absl/meta:type_traits", - "absl/utility", - ], language = "c++", public_hdrs = [ "lib/promise/detail/basic_seq.h", ], + deps = [ + "construct_destruct", + "poll", + "//:gpr_platform", + ], +) + +grpc_cc_library( + name = "seq_state", + external_deps = ["absl/base:core_headers"], + language = "c++", + public_hdrs = [ + "lib/promise/detail/seq_state.h", + ], deps = [ "construct_destruct", "poll", "promise_factory", "promise_like", - "switch", - "//:gpr_platform", + "//:gpr", ], ) @@ -693,6 +702,7 @@ grpc_cc_library( "basic_seq", "poll", "promise_like", + "seq_state", "//:gpr_platform", ], ) @@ -713,6 +723,7 @@ grpc_cc_library( "poll", "promise_like", "promise_status", + "seq_state", "//:gpr_platform", ], ) @@ -1080,7 +1091,6 @@ grpc_cc_library( ], deps = [ "activity", - "basic_seq", "event_engine_memory_allocator", "exec_ctx_wakeup_scheduler", "experiments", diff --git a/src/core/ext/filters/channel_idle/channel_idle_filter.cc b/src/core/ext/filters/channel_idle/channel_idle_filter.cc index 34601e5e3a0..5a7d98efc47 100644 --- a/src/core/ext/filters/channel_idle/channel_idle_filter.cc +++ b/src/core/ext/filters/channel_idle/channel_idle_filter.cc @@ -19,6 +19,7 @@ #include "src/core/ext/filters/channel_idle/channel_idle_filter.h" +#include #include #include diff --git a/src/core/lib/channel/connected_channel.cc b/src/core/lib/channel/connected_channel.cc index 83dcb93845b..29f0924b665 100644 --- a/src/core/lib/channel/connected_channel.cc +++ b/src/core/lib/channel/connected_channel.cc @@ -57,7 +57,6 @@ #include "src/core/lib/promise/activity.h" #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/promise/context.h" -#include "src/core/lib/promise/detail/basic_seq.h" #include "src/core/lib/promise/detail/status.h" #include "src/core/lib/promise/for_each.h" #include "src/core/lib/promise/if.h" diff --git a/src/core/lib/channel/promise_based_filter.cc b/src/core/lib/channel/promise_based_filter.cc index 27f3e5bca8d..f81bcc08ca2 100644 --- a/src/core/lib/channel/promise_based_filter.cc +++ b/src/core/lib/channel/promise_based_filter.cc @@ -38,7 +38,7 @@ #include "src/core/lib/gprpp/manual_constructor.h" #include "src/core/lib/gprpp/status_helper.h" #include "src/core/lib/iomgr/error.h" -#include "src/core/lib/promise/detail/basic_seq.h" +#include "src/core/lib/promise/seq.h" #include "src/core/lib/slice/slice.h" extern grpc_core::TraceFlag grpc_trace_channel; diff --git a/src/core/lib/promise/detail/basic_seq.h b/src/core/lib/promise/detail/basic_seq.h index 3ebb251a961..663609d4192 100644 --- a/src/core/lib/promise/detail/basic_seq.h +++ b/src/core/lib/promise/detail/basic_seq.h @@ -17,384 +17,13 @@ #include -#include -#include -#include -#include -#include - -#include "absl/meta/type_traits.h" -#include "absl/utility/utility.h" - #include "src/core/lib/gprpp/construct_destruct.h" -#include "src/core/lib/promise/detail/promise_factory.h" -#include "src/core/lib/promise/detail/promise_like.h" -#include "src/core/lib/promise/detail/switch.h" #include "src/core/lib/promise/poll.h" namespace grpc_core { namespace promise_detail { -// Helper for SeqState to evaluate some common types to all partial -// specializations. -template