From c0e6701c57de49e3134ca2a940399f39380ba1d0 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Fri, 17 Mar 2017 20:00:55 -0700 Subject: [PATCH 1/3] Added dependencies in build.yaml to allow building --- CMakeLists.txt | 2 ++ Makefile | 18 +++++++++--------- build.yaml | 3 +++ grpc.def | 1 + src/ruby/ext/grpc/rb_grpc_imports.generated.c | 2 ++ src/ruby/ext/grpc/rb_grpc_imports.generated.h | 3 +++ .../generated/sources_and_headers.json | 2 ++ .../codegen_test_minimal.vcxproj | 8 ++++++++ 8 files changed, 30 insertions(+), 9 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 180f4166091..40db69f1d98 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8424,6 +8424,8 @@ target_include_directories(codegen_test_minimal target_link_libraries(codegen_test_minimal ${_gRPC_PROTOBUF_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} + grpc + gpr ${_gRPC_GFLAGS_LIBRARIES} ) diff --git a/Makefile b/Makefile index 2a9cbc382bb..32bf2ab0bcd 100644 --- a/Makefile +++ b/Makefile @@ -13505,28 +13505,28 @@ $(BINDIR)/$(CONFIG)/codegen_test_minimal: protobuf_dep_error else -$(BINDIR)/$(CONFIG)/codegen_test_minimal: $(PROTOBUF_DEP) $(CODEGEN_TEST_MINIMAL_OBJS) +$(BINDIR)/$(CONFIG)/codegen_test_minimal: $(PROTOBUF_DEP) $(CODEGEN_TEST_MINIMAL_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a $(E) "[LD] Linking $@" $(Q) mkdir -p `dirname $@` - $(Q) $(LDXX) $(LDFLAGS) $(CODEGEN_TEST_MINIMAL_OBJS) $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/codegen_test_minimal + $(Q) $(LDXX) $(LDFLAGS) $(CODEGEN_TEST_MINIMAL_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/codegen_test_minimal endif endif -$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/control.o: +$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/control.o: $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a -$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/messages.o: +$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/messages.o: $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a -$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/payloads.o: +$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/payloads.o: $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a -$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/services.o: +$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/services.o: $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a -$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/stats.o: +$(OBJDIR)/$(CONFIG)/src/proto/grpc/testing/stats.o: $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a -$(OBJDIR)/$(CONFIG)/test/cpp/codegen/codegen_test_minimal.o: +$(OBJDIR)/$(CONFIG)/test/cpp/codegen/codegen_test_minimal.o: $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a -$(OBJDIR)/$(CONFIG)/src/cpp/codegen/codegen_init.o: +$(OBJDIR)/$(CONFIG)/src/cpp/codegen/codegen_init.o: $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr.a deps_codegen_test_minimal: $(CODEGEN_TEST_MINIMAL_OBJS:.o=.dep) diff --git a/build.yaml b/build.yaml index 997f55df97a..af415f63c27 100644 --- a/build.yaml +++ b/build.yaml @@ -3413,6 +3413,9 @@ targets: - src/proto/grpc/testing/services.proto - src/proto/grpc/testing/stats.proto - test/cpp/codegen/codegen_test_minimal.cc + deps: + - grpc + - gpr filegroups: - grpc++_codegen_base - grpc++_codegen_base_src diff --git a/grpc.def b/grpc.def index 30d60b0d06d..1022e5b7f10 100644 --- a/grpc.def +++ b/grpc.def @@ -67,6 +67,7 @@ EXPORTS grpc_channel_ping grpc_channel_register_call grpc_channel_create_registered_call + grpc_call_arena_alloc grpc_call_start_batch grpc_call_get_peer grpc_call_set_load_reporting_cost_context diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c index 3ef6f0eb299..8afe1e3d36b 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c @@ -105,6 +105,7 @@ grpc_channel_create_call_type grpc_channel_create_call_import; grpc_channel_ping_type grpc_channel_ping_import; grpc_channel_register_call_type grpc_channel_register_call_import; grpc_channel_create_registered_call_type grpc_channel_create_registered_call_import; +grpc_call_arena_alloc_type grpc_call_arena_alloc_import; grpc_call_start_batch_type grpc_call_start_batch_import; grpc_call_get_peer_type grpc_call_get_peer_import; grpc_call_set_load_reporting_cost_context_type grpc_call_set_load_reporting_cost_context_import; @@ -399,6 +400,7 @@ void grpc_rb_load_imports(HMODULE library) { grpc_channel_ping_import = (grpc_channel_ping_type) GetProcAddress(library, "grpc_channel_ping"); grpc_channel_register_call_import = (grpc_channel_register_call_type) GetProcAddress(library, "grpc_channel_register_call"); grpc_channel_create_registered_call_import = (grpc_channel_create_registered_call_type) GetProcAddress(library, "grpc_channel_create_registered_call"); + grpc_call_arena_alloc_import = (grpc_call_arena_alloc_type) GetProcAddress(library, "grpc_call_arena_alloc"); grpc_call_start_batch_import = (grpc_call_start_batch_type) GetProcAddress(library, "grpc_call_start_batch"); grpc_call_get_peer_import = (grpc_call_get_peer_type) GetProcAddress(library, "grpc_call_get_peer"); grpc_call_set_load_reporting_cost_context_import = (grpc_call_set_load_reporting_cost_context_type) GetProcAddress(library, "grpc_call_set_load_reporting_cost_context"); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h index ef9845dfe06..9778f7816f6 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h @@ -266,6 +266,9 @@ extern grpc_channel_register_call_type grpc_channel_register_call_import; typedef grpc_call *(*grpc_channel_create_registered_call_type)(grpc_channel *channel, grpc_call *parent_call, uint32_t propagation_mask, grpc_completion_queue *completion_queue, void *registered_call_handle, gpr_timespec deadline, void *reserved); extern grpc_channel_create_registered_call_type grpc_channel_create_registered_call_import; #define grpc_channel_create_registered_call grpc_channel_create_registered_call_import +typedef void *(*grpc_call_arena_alloc_type)(grpc_call *call, size_t size); +extern grpc_call_arena_alloc_type grpc_call_arena_alloc_import; +#define grpc_call_arena_alloc grpc_call_arena_alloc_import typedef grpc_call_error(*grpc_call_start_batch_type)(grpc_call *call, const grpc_op *ops, size_t nops, void *tag, void *reserved); extern grpc_call_start_batch_type grpc_call_start_batch_import; #define grpc_call_start_batch grpc_call_start_batch_import diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 866d8a55c96..ca23d1d99da 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -2816,6 +2816,8 @@ }, { "deps": [ + "gpr", + "grpc", "grpc++_codegen_base", "grpc++_codegen_base_src" ], diff --git a/vsprojects/vcxproj/test/codegen_test_minimal/codegen_test_minimal.vcxproj b/vsprojects/vcxproj/test/codegen_test_minimal/codegen_test_minimal.vcxproj index fd014fdc098..d176a36f208 100644 --- a/vsprojects/vcxproj/test/codegen_test_minimal/codegen_test_minimal.vcxproj +++ b/vsprojects/vcxproj/test/codegen_test_minimal/codegen_test_minimal.vcxproj @@ -256,6 +256,14 @@ + + + {29D16885-7228-4C31-81ED-5F9187C7F2A9} + + + {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} + + From 379be59d0aa65e88ac3754bb3dc4635931263714 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Fri, 17 Mar 2017 20:36:31 -0700 Subject: [PATCH 2/3] Pour one out for shared ptr --- .../grpc++/impl/codegen/async_unary_call.h | 12 +++-- include/grpc++/impl/codegen/call.h | 50 ++++++++++++++----- 2 files changed, 47 insertions(+), 15 deletions(-) diff --git a/include/grpc++/impl/codegen/async_unary_call.h b/include/grpc++/impl/codegen/async_unary_call.h index d11986b6ece..50bb399df62 100644 --- a/include/grpc++/impl/codegen/async_unary_call.h +++ b/include/grpc++/impl/codegen/async_unary_call.h @@ -61,6 +61,11 @@ template class ClientAsyncResponseReader final : public ClientAsyncResponseReaderInterface { public: + ~ClientAsyncResponseReader() { + if (collection_ != nullptr && collection_->Unref()) { + delete collection_; + } + } template static ClientAsyncResponseReader* Create(ChannelInterface* channel, CompletionQueue* cq, @@ -72,9 +77,10 @@ class ClientAsyncResponseReader final grpc_call_arena_alloc(call.call(), sizeof(*reader))); new (&reader->call_) Call(std::move(call)); reader->context_ = context; - new (&reader->collection_) std::shared_ptr( + reader->collection_ = new (grpc_call_arena_alloc(call.call(), sizeof(CallOpSetCollection))) - CallOpSetCollection()); + CallOpSetCollection(); + reader->collection_->init_buf_.SetCollection(reader->collection_); reader->collection_->init_buf_.SendInitialMetadata( context->send_initial_metadata_, context->initial_metadata_flags()); @@ -138,7 +144,7 @@ class ClientAsyncResponseReader final // disable operator new static void* operator new(std::size_t size); }; - std::shared_ptr collection_; + CallOpSetCollection* collection_; }; template diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 19a5ca2b2ee..69fe21d0240 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -34,6 +34,7 @@ #ifndef GRPCXX_IMPL_CODEGEN_CALL_H #define GRPCXX_IMPL_CODEGEN_CALL_H +#include #include #include #include @@ -50,6 +51,7 @@ #include #include +#include #include #include @@ -523,14 +525,30 @@ class CallOpClientRecvStatus { /// An abstract collection of CallOpSet's, to be used whenever /// CallOpSet objects must be thought of as a group. Each member -/// of the group should have a shared_ptr back to the collection, -/// as will the object that instantiates the collection, allowing -/// for automatic ref-counting. In practice, any actual use should -/// derive from this base class. This is specifically necessary if -/// some of the CallOpSet's in the collection are "Sneaky" and don't -/// report back to the C++ layer CQ operations -class CallOpSetCollectionInterface - : public std::enable_shared_from_this {}; +/// of the group should reference the collection, as will the object +/// that instantiates the collection, allowing for ref-counting. +/// Any actual use should derive from this base class. This is specifically +/// necessary if some of the CallOpSet's in the collection are "Sneaky" and +/// don't report back to the C++ layer CQ operations +class CallOpSetCollectionInterface { + public: + CallOpSetCollectionInterface() { + gpr_atm_rel_store(&refs_, static_cast(1)); + } + // always allocated against a call arena, no memory free required + static void operator delete(void* ptr, std::size_t size) { + assert(size == sizeof(CallOpSetCollectionInterface)); + } + void Ref() { gpr_atm_no_barrier_fetch_add(&refs_, static_cast(1)); } + bool Unref() { + gpr_atm old = + gpr_atm_no_barrier_fetch_add(&refs_, static_cast(-1)); + return (old == static_cast(1)); + } + + private: + gpr_atm refs_; +}; /// An abstract collection of call ops, used to generate the /// grpc_call_op structure to pass down to the lower layers, @@ -539,18 +557,26 @@ class CallOpSetCollectionInterface /// API. class CallOpSetInterface : public CompletionQueueTag { public: - CallOpSetInterface() {} + CallOpSetInterface() : collection_(nullptr) {} + ~CallOpSetInterface() { ResetCollection(); } /// Fills in grpc_op, starting from ops[*nops] and moving /// upwards. virtual void FillOps(grpc_op* ops, size_t* nops) = 0; /// Mark this as belonging to a collection if needed - void SetCollection(std::shared_ptr collection) { + void SetCollection(CallOpSetCollectionInterface* collection) { collection_ = collection; + collection->Ref(); + } + void ResetCollection() { + if (collection_ != nullptr && collection_->Unref()) { + delete collection_; + } + collection_ = nullptr; } protected: - std::shared_ptr collection_; + CallOpSetCollectionInterface* collection_; }; /// Primary implementaiton of CallOpSetInterface. @@ -588,7 +614,7 @@ class CallOpSet : public CallOpSetInterface, this->Op5::FinishOp(status); this->Op6::FinishOp(status); *tag = return_tag_; - collection_.reset(); // drop the ref at this point + ResetCollection(); // drop the ref at this point return true; } From 60a41907a0bbb24be89a421ca8ba8fb60a30e773 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Tue, 21 Mar 2017 09:39:11 -0700 Subject: [PATCH 3/3] Remove delete assertion on base class and change to full fetch-add --- include/grpc++/impl/codegen/call.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 69fe21d0240..04296dbe15e 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -537,12 +537,11 @@ class CallOpSetCollectionInterface { } // always allocated against a call arena, no memory free required static void operator delete(void* ptr, std::size_t size) { - assert(size == sizeof(CallOpSetCollectionInterface)); } void Ref() { gpr_atm_no_barrier_fetch_add(&refs_, static_cast(1)); } bool Unref() { gpr_atm old = - gpr_atm_no_barrier_fetch_add(&refs_, static_cast(-1)); + gpr_atm_full_fetch_add(&refs_, static_cast(-1)); return (old == static_cast(1)); }