From f13a74312672aaf7c72e984f59dbd351dcda4e8a Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 26 Sep 2018 17:19:39 -0700 Subject: [PATCH 1/4] Implement child socket support --- .../client_channel/client_channel_channelz.cc | 19 +++++++++++++++++++ .../client_channel/client_channel_channelz.h | 8 +++----- .../ext/filters/client_channel/subchannel.cc | 10 ++++++++++ .../ext/filters/client_channel/subchannel.h | 3 +++ .../chttp2/transport/chttp2_transport.cc | 12 +++++++++++- .../cronet/transport/cronet_transport.cc | 6 +++++- .../ext/transport/inproc/inproc_transport.cc | 5 ++++- src/core/lib/transport/transport.cc | 5 +++++ src/core/lib/transport/transport.h | 10 ++++++++++ src/core/lib/transport/transport_impl.h | 3 +++ 10 files changed, 73 insertions(+), 8 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index 7e8f59bcd33..4fedcbcbb6c 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -136,6 +136,23 @@ void SubchannelNode::PopulateConnectivityState(grpc_json* json) { false); } +void SubchannelNode::PopulateChildSockets(grpc_json* json) { + ChildRefsList child_sockets; + grpc_json* json_iterator = nullptr; + grpc_subchannel_populate_child_sockets(subchannel_, &child_sockets); + if (!child_sockets.empty()) { + grpc_json* array_parent = grpc_json_create_child( + nullptr, json, "socketRef", nullptr, GRPC_JSON_ARRAY, false); + for (size_t i = 0; i < child_sockets.size(); ++i) { + json_iterator = + grpc_json_create_child(json_iterator, array_parent, nullptr, nullptr, + GRPC_JSON_OBJECT, false); + grpc_json_add_number_string_child(json_iterator, nullptr, "socketId", + child_sockets[i]); + } + } +} + grpc_json* SubchannelNode::RenderJson() { grpc_json* top_level_json = grpc_json_create(GRPC_JSON_OBJECT); grpc_json* json = top_level_json; @@ -166,6 +183,8 @@ grpc_json* SubchannelNode::RenderJson() { } // ask CallCountingHelper to populate trace and call count data. call_counter_.PopulateCallCounts(json); + json = top_level_json; + PopulateChildSockets(json); return top_level_json; } diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h index 8ce331e529d..a63df00f9f2 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.h +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -31,11 +31,6 @@ typedef struct grpc_subchannel grpc_subchannel; namespace grpc_core { -// TODO(ncteisen), this only contains the uuids of the children for now, -// since that is all that is strictly needed. In a future enhancement we will -// add human readable names as in the channelz.proto -typedef InlinedVector ChildRefsList; - namespace channelz { // Subtype of ChannelNode that overrides and provides client_channel specific @@ -76,6 +71,9 @@ class SubchannelNode : public BaseNode { grpc_json* RenderJson() override; + // helper to populate the socket(s) that this subchannel owns. + void PopulateChildSockets(grpc_json* json); + // proxy methods to composed classes. void AddTraceEvent(ChannelTrace::Severity severity, grpc_slice data) { trace_.AddTraceEvent(severity, data); diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 57d0b3759f2..29aeb06e7bc 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -97,6 +97,8 @@ struct grpc_subchannel { /** set during connection */ grpc_connect_out_args connecting_result; + grpc_transport* transport; + /** callback for connection finishing */ grpc_closure on_connected; @@ -411,6 +413,13 @@ grpc_core::channelz::SubchannelNode* grpc_subchannel_get_channelz_node( return subchannel->channelz_subchannel.get(); } +void grpc_subchannel_populate_child_sockets( + grpc_subchannel* subchannel, grpc_core::ChildRefsList* child_sockets) { + if (subchannel->transport != nullptr) { + grpc_transport_populate_sockets(subchannel->transport, child_sockets); + } +} + static void continue_connect_locked(grpc_subchannel* c) { grpc_connect_in_args args; args.interested_parties = c->pollset_set; @@ -621,6 +630,7 @@ static bool publish_transport_locked(grpc_subchannel* c) { GRPC_ERROR_UNREF(error); return false; } + c->transport = c->connecting_result.transport; memset(&c->connecting_result, 0, sizeof(c->connecting_result)); /* initialize state watcher */ diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h index 84febb52040..2cf5067fc26 100644 --- a/src/core/ext/filters/client_channel/subchannel.h +++ b/src/core/ext/filters/client_channel/subchannel.h @@ -126,6 +126,9 @@ void grpc_subchannel_call_unref( grpc_core::channelz::SubchannelNode* grpc_subchannel_get_channelz_node( grpc_subchannel* subchannel); +void grpc_subchannel_populate_child_sockets( + grpc_subchannel* subchannel, grpc_core::ChildRefsList* child_sockets); + /** Returns a pointer to the parent data associated with \a subchannel_call. The data will be of the size specified in \a parent_data_size field of the args passed to \a grpc_connected_subchannel_create_call(). */ diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index d3232f4d268..202da652de7 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -3157,6 +3157,15 @@ static grpc_endpoint* chttp2_get_endpoint(grpc_transport* t) { return (reinterpret_cast(t))->ep; } +static void populate_sockets(grpc_transport* transport, + grpc_core::ChildRefsList* child_sockets) { + grpc_chttp2_transport* t = + reinterpret_cast(transport); + if (t->channelz_socket != nullptr) { + child_sockets->push_back(t->channelz_socket->uuid()); + } +} + static const grpc_transport_vtable vtable = {sizeof(grpc_chttp2_stream), "chttp2", init_stream, @@ -3166,7 +3175,8 @@ static const grpc_transport_vtable vtable = {sizeof(grpc_chttp2_stream), perform_transport_op, destroy_stream, destroy_transport, - chttp2_get_endpoint}; + chttp2_get_endpoint, + populate_sockets}; static const grpc_transport_vtable* get_vtable(void) { return &vtable; } diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc index 81e2634e3a7..f3826a78de0 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.cc +++ b/src/core/ext/transport/cronet/transport/cronet_transport.cc @@ -1439,6 +1439,9 @@ static grpc_endpoint* get_endpoint(grpc_transport* gt) { return nullptr; } static void perform_op(grpc_transport* gt, grpc_transport_op* op) {} +static void populate_sockets(grpc_transport* t, + grpc_core::ChildRefsList* child_sockets) {} + static const grpc_transport_vtable grpc_cronet_vtable = { sizeof(stream_obj), "cronet_http", @@ -1449,7 +1452,8 @@ static const grpc_transport_vtable grpc_cronet_vtable = { perform_op, destroy_stream, destroy_transport, - get_endpoint}; + get_endpoint, + populate_sockets}; grpc_transport* grpc_create_cronet_transport(void* engine, const char* target, const grpc_channel_args* args, diff --git a/src/core/ext/transport/inproc/inproc_transport.cc b/src/core/ext/transport/inproc/inproc_transport.cc index b0ca7f8207e..2deaacb2e06 100644 --- a/src/core/ext/transport/inproc/inproc_transport.cc +++ b/src/core/ext/transport/inproc/inproc_transport.cc @@ -1170,6 +1170,9 @@ static void set_pollset_set(grpc_transport* gt, grpc_stream* gs, static grpc_endpoint* get_endpoint(grpc_transport* t) { return nullptr; } +static void populate_sockets(grpc_transport* t, + grpc_core::ChildRefsList* child_sockets) {} + /******************************************************************************* * GLOBAL INIT AND DESTROY */ @@ -1194,7 +1197,7 @@ static const grpc_transport_vtable inproc_vtable = { sizeof(inproc_stream), "inproc", init_stream, set_pollset, set_pollset_set, perform_stream_op, perform_transport_op, destroy_stream, destroy_transport, - get_endpoint}; + get_endpoint, populate_sockets}; /******************************************************************************* * Main inproc transport functions diff --git a/src/core/lib/transport/transport.cc b/src/core/lib/transport/transport.cc index cbdb77c8441..2e7c40dda45 100644 --- a/src/core/lib/transport/transport.cc +++ b/src/core/lib/transport/transport.cc @@ -199,6 +199,11 @@ grpc_endpoint* grpc_transport_get_endpoint(grpc_transport* transport) { return transport->vtable->get_endpoint(transport); } +void grpc_transport_populate_sockets(grpc_transport* transport, + grpc_core::ChildRefsList* child_sockets) { + return transport->vtable->populate_sockets(transport, child_sockets); +} + // This comment should be sung to the tune of // "Supercalifragilisticexpialidocious": // diff --git a/src/core/lib/transport/transport.h b/src/core/lib/transport/transport.h index 9e784635c69..93445ebd051 100644 --- a/src/core/lib/transport/transport.h +++ b/src/core/lib/transport/transport.h @@ -39,6 +39,13 @@ #define GRPC_PROTOCOL_VERSION_MIN_MAJOR 2 #define GRPC_PROTOCOL_VERSION_MIN_MINOR 1 +namespace grpc_core { +// TODO(ncteisen), this only contains the uuids of the children for now, +// since that is all that is strictly needed. In a future enhancement we will +// add human readable names as in the channelz.proto +typedef InlinedVector ChildRefsList; +} // namespace grpc_core + /* forward declarations */ typedef struct grpc_transport grpc_transport; @@ -366,6 +373,9 @@ void grpc_transport_destroy(grpc_transport* transport); /* Get the endpoint used by \a transport */ grpc_endpoint* grpc_transport_get_endpoint(grpc_transport* transport); +void grpc_transport_populate_sockets(grpc_transport* transport, + grpc_core::ChildRefsList* child_sockets); + /* Allocate a grpc_transport_op, and preconfigure the on_consumed closure to \a on_consumed and then delete the returned transport op */ grpc_transport_op* grpc_make_transport_op(grpc_closure* on_consumed); diff --git a/src/core/lib/transport/transport_impl.h b/src/core/lib/transport/transport_impl.h index ba5e05df0af..7ae59a1d17f 100644 --- a/src/core/lib/transport/transport_impl.h +++ b/src/core/lib/transport/transport_impl.h @@ -60,6 +60,9 @@ typedef struct grpc_transport_vtable { /* implementation of grpc_transport_get_endpoint */ grpc_endpoint* (*get_endpoint)(grpc_transport* self); + + void (*populate_sockets)(grpc_transport* self, + grpc_core::ChildRefsList* child_sockets); } grpc_transport_vtable; /* an instance of a grpc transport */ From 4cc16f951c0909196a9ed62774adcbbaf9cc88c1 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 27 Sep 2018 09:45:59 -0500 Subject: [PATCH 2/4] Simplifiy transport querying function --- .../client_channel/client_channel_channelz.cc | 29 +++++++------------ .../client_channel/client_channel_channelz.h | 5 ++++ .../ext/filters/client_channel/subchannel.cc | 16 +++++----- .../ext/filters/client_channel/subchannel.h | 4 +-- .../chttp2/transport/chttp2_transport.cc | 9 +++--- .../cronet/transport/cronet_transport.cc | 5 ++-- .../ext/transport/inproc/inproc_transport.cc | 5 ++-- src/core/lib/transport/transport.cc | 5 ++-- src/core/lib/transport/transport.h | 10 +------ src/core/lib/transport/transport_impl.h | 3 +- test/cpp/microbenchmarks/bm_call_create.cc | 10 ++++--- 11 files changed, 46 insertions(+), 55 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index 4fedcbcbb6c..8d02304d19f 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -136,23 +136,6 @@ void SubchannelNode::PopulateConnectivityState(grpc_json* json) { false); } -void SubchannelNode::PopulateChildSockets(grpc_json* json) { - ChildRefsList child_sockets; - grpc_json* json_iterator = nullptr; - grpc_subchannel_populate_child_sockets(subchannel_, &child_sockets); - if (!child_sockets.empty()) { - grpc_json* array_parent = grpc_json_create_child( - nullptr, json, "socketRef", nullptr, GRPC_JSON_ARRAY, false); - for (size_t i = 0; i < child_sockets.size(); ++i) { - json_iterator = - grpc_json_create_child(json_iterator, array_parent, nullptr, nullptr, - GRPC_JSON_OBJECT, false); - grpc_json_add_number_string_child(json_iterator, nullptr, "socketId", - child_sockets[i]); - } - } -} - grpc_json* SubchannelNode::RenderJson() { grpc_json* top_level_json = grpc_json_create(GRPC_JSON_OBJECT); grpc_json* json = top_level_json; @@ -184,7 +167,17 @@ grpc_json* SubchannelNode::RenderJson() { // ask CallCountingHelper to populate trace and call count data. call_counter_.PopulateCallCounts(json); json = top_level_json; - PopulateChildSockets(json); + // populate the child socket. + intptr_t socket_uuid = grpc_subchannel_get_child_socket_uuid(subchannel_); + if (socket_uuid != 0) { + grpc_json* array_parent = grpc_json_create_child( + nullptr, json, "socketRef", nullptr, GRPC_JSON_ARRAY, false); + json_iterator = + grpc_json_create_child(json_iterator, array_parent, nullptr, nullptr, + GRPC_JSON_OBJECT, false); + grpc_json_add_number_string_child(json_iterator, nullptr, "socketId", + socket_uuid); + } return top_level_json; } diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h index a63df00f9f2..a6dae165223 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.h +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -31,6 +31,11 @@ typedef struct grpc_subchannel grpc_subchannel; namespace grpc_core { +// TODO(ncteisen), this only contains the uuids of the children for now, +// since that is all that is strictly needed. In a future enhancement we will +// add human readable names as in the channelz.proto +typedef InlinedVector ChildRefsList; + namespace channelz { // Subtype of ChannelNode that overrides and provides client_channel specific diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 29aeb06e7bc..4c33636646c 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -97,7 +97,9 @@ struct grpc_subchannel { /** set during connection */ grpc_connect_out_args connecting_result; - grpc_transport* transport; + /** uuid of this subchannel's socket. 0 if this subchannel is not + connected */ + intptr_t socket_uuid; /** callback for connection finishing */ grpc_closure on_connected; @@ -256,6 +258,7 @@ static void disconnect(grpc_subchannel* c) { c->disconnected = true; grpc_connector_shutdown(c->connector, GRPC_ERROR_CREATE_FROM_STATIC_STRING( "Subchannel disconnected")); + c->socket_uuid = 0; c->connected_subchannel.reset(); gpr_mu_unlock(&c->mu); } @@ -413,11 +416,9 @@ grpc_core::channelz::SubchannelNode* grpc_subchannel_get_channelz_node( return subchannel->channelz_subchannel.get(); } -void grpc_subchannel_populate_child_sockets( - grpc_subchannel* subchannel, grpc_core::ChildRefsList* child_sockets) { - if (subchannel->transport != nullptr) { - grpc_transport_populate_sockets(subchannel->transport, child_sockets); - } +intptr_t grpc_subchannel_get_child_socket_uuid( + grpc_subchannel* subchannel) { + return subchannel->socket_uuid; } static void continue_connect_locked(grpc_subchannel* c) { @@ -578,6 +579,7 @@ static void on_connected_subchannel_connectivity_changed(void* p, grpc_connectivity_state_name( connected_subchannel_watcher->connectivity_state)); } + c->socket_uuid = 0; c->connected_subchannel.reset(); grpc_connectivity_state_set(&c->state_tracker, GRPC_CHANNEL_TRANSIENT_FAILURE, @@ -630,7 +632,7 @@ static bool publish_transport_locked(grpc_subchannel* c) { GRPC_ERROR_UNREF(error); return false; } - c->transport = c->connecting_result.transport; + c->socket_uuid = grpc_transport_get_socket_uuid(c->connecting_result.transport); memset(&c->connecting_result, 0, sizeof(c->connecting_result)); /* initialize state watcher */ diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h index 2cf5067fc26..67a2d497111 100644 --- a/src/core/ext/filters/client_channel/subchannel.h +++ b/src/core/ext/filters/client_channel/subchannel.h @@ -126,8 +126,8 @@ void grpc_subchannel_call_unref( grpc_core::channelz::SubchannelNode* grpc_subchannel_get_channelz_node( grpc_subchannel* subchannel); -void grpc_subchannel_populate_child_sockets( - grpc_subchannel* subchannel, grpc_core::ChildRefsList* child_sockets); +intptr_t grpc_subchannel_get_child_socket_uuid( + grpc_subchannel* subchannel); /** Returns a pointer to the parent data associated with \a subchannel_call. The data will be of the size specified in \a parent_data_size diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 202da652de7..45ef4161f78 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -3157,12 +3157,13 @@ static grpc_endpoint* chttp2_get_endpoint(grpc_transport* t) { return (reinterpret_cast(t))->ep; } -static void populate_sockets(grpc_transport* transport, - grpc_core::ChildRefsList* child_sockets) { +static intptr_t get_socket_uuid(grpc_transport* transport) { grpc_chttp2_transport* t = reinterpret_cast(transport); if (t->channelz_socket != nullptr) { - child_sockets->push_back(t->channelz_socket->uuid()); + return t->channelz_socket->uuid(); + } else { + return 0; } } @@ -3176,7 +3177,7 @@ static const grpc_transport_vtable vtable = {sizeof(grpc_chttp2_stream), destroy_stream, destroy_transport, chttp2_get_endpoint, - populate_sockets}; + get_socket_uuid}; static const grpc_transport_vtable* get_vtable(void) { return &vtable; } diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc index f3826a78de0..bb20d9db084 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.cc +++ b/src/core/ext/transport/cronet/transport/cronet_transport.cc @@ -1439,8 +1439,7 @@ static grpc_endpoint* get_endpoint(grpc_transport* gt) { return nullptr; } static void perform_op(grpc_transport* gt, grpc_transport_op* op) {} -static void populate_sockets(grpc_transport* t, - grpc_core::ChildRefsList* child_sockets) {} +static intptr_t get_socket_uuid(grpc_transport* t) { return 0; } static const grpc_transport_vtable grpc_cronet_vtable = { sizeof(stream_obj), @@ -1453,7 +1452,7 @@ static const grpc_transport_vtable grpc_cronet_vtable = { destroy_stream, destroy_transport, get_endpoint, - populate_sockets}; + get_socket_uuid}; grpc_transport* grpc_create_cronet_transport(void* engine, const char* target, const grpc_channel_args* args, diff --git a/src/core/ext/transport/inproc/inproc_transport.cc b/src/core/ext/transport/inproc/inproc_transport.cc index 2deaacb2e06..56951f83386 100644 --- a/src/core/ext/transport/inproc/inproc_transport.cc +++ b/src/core/ext/transport/inproc/inproc_transport.cc @@ -1170,8 +1170,7 @@ static void set_pollset_set(grpc_transport* gt, grpc_stream* gs, static grpc_endpoint* get_endpoint(grpc_transport* t) { return nullptr; } -static void populate_sockets(grpc_transport* t, - grpc_core::ChildRefsList* child_sockets) {} +static intptr_t get_socket_uuid(grpc_transport* t) { return 0; } /******************************************************************************* * GLOBAL INIT AND DESTROY @@ -1197,7 +1196,7 @@ static const grpc_transport_vtable inproc_vtable = { sizeof(inproc_stream), "inproc", init_stream, set_pollset, set_pollset_set, perform_stream_op, perform_transport_op, destroy_stream, destroy_transport, - get_endpoint, populate_sockets}; + get_endpoint, get_socket_uuid}; /******************************************************************************* * Main inproc transport functions diff --git a/src/core/lib/transport/transport.cc b/src/core/lib/transport/transport.cc index 2e7c40dda45..c8fdbb4de26 100644 --- a/src/core/lib/transport/transport.cc +++ b/src/core/lib/transport/transport.cc @@ -199,9 +199,8 @@ grpc_endpoint* grpc_transport_get_endpoint(grpc_transport* transport) { return transport->vtable->get_endpoint(transport); } -void grpc_transport_populate_sockets(grpc_transport* transport, - grpc_core::ChildRefsList* child_sockets) { - return transport->vtable->populate_sockets(transport, child_sockets); +intptr_t grpc_transport_get_socket_uuid(grpc_transport* transport) { + return transport->vtable->get_socket_uuid(transport); } // This comment should be sung to the tune of diff --git a/src/core/lib/transport/transport.h b/src/core/lib/transport/transport.h index 93445ebd051..aad133f6c3d 100644 --- a/src/core/lib/transport/transport.h +++ b/src/core/lib/transport/transport.h @@ -39,13 +39,6 @@ #define GRPC_PROTOCOL_VERSION_MIN_MAJOR 2 #define GRPC_PROTOCOL_VERSION_MIN_MINOR 1 -namespace grpc_core { -// TODO(ncteisen), this only contains the uuids of the children for now, -// since that is all that is strictly needed. In a future enhancement we will -// add human readable names as in the channelz.proto -typedef InlinedVector ChildRefsList; -} // namespace grpc_core - /* forward declarations */ typedef struct grpc_transport grpc_transport; @@ -373,8 +366,7 @@ void grpc_transport_destroy(grpc_transport* transport); /* Get the endpoint used by \a transport */ grpc_endpoint* grpc_transport_get_endpoint(grpc_transport* transport); -void grpc_transport_populate_sockets(grpc_transport* transport, - grpc_core::ChildRefsList* child_sockets); +intptr_t grpc_transport_get_socket_uuid(grpc_transport* transport); /* Allocate a grpc_transport_op, and preconfigure the on_consumed closure to \a on_consumed and then delete the returned transport op */ diff --git a/src/core/lib/transport/transport_impl.h b/src/core/lib/transport/transport_impl.h index 7ae59a1d17f..d609470ef01 100644 --- a/src/core/lib/transport/transport_impl.h +++ b/src/core/lib/transport/transport_impl.h @@ -61,8 +61,7 @@ typedef struct grpc_transport_vtable { /* implementation of grpc_transport_get_endpoint */ grpc_endpoint* (*get_endpoint)(grpc_transport* self); - void (*populate_sockets)(grpc_transport* self, - grpc_core::ChildRefsList* child_sockets); + intptr_t (*get_socket_uuid)(grpc_transport* self); } grpc_transport_vtable; /* an instance of a grpc transport */ diff --git a/test/cpp/microbenchmarks/bm_call_create.cc b/test/cpp/microbenchmarks/bm_call_create.cc index 9516b2e3e25..b0cccafcf82 100644 --- a/test/cpp/microbenchmarks/bm_call_create.cc +++ b/test/cpp/microbenchmarks/bm_call_create.cc @@ -446,11 +446,13 @@ void Destroy(grpc_transport* self) {} /* implementation of grpc_transport_get_endpoint */ grpc_endpoint* GetEndpoint(grpc_transport* self) { return nullptr; } +intptr_t GetSocketUuid(grpc_transport* t) { return 0; } + static const grpc_transport_vtable dummy_transport_vtable = { - 0, "dummy_http2", InitStream, - SetPollset, SetPollsetSet, PerformStreamOp, - PerformOp, DestroyStream, Destroy, - GetEndpoint}; + 0, "dummy_http2", InitStream, + SetPollset, SetPollsetSet, PerformStreamOp, + PerformOp, DestroyStream, Destroy, + GetEndpoint, GetSocketUuid}; static grpc_transport dummy_transport = {&dummy_transport_vtable}; From f2b493e3697211552a28dba582313174f2a932f2 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 27 Sep 2018 10:38:37 -0500 Subject: [PATCH 3/4] reviewer feedback --- .../client_channel/client_channel_channelz.cc | 5 ++-- .../client_channel/client_channel_channelz.h | 3 --- .../ext/filters/client_channel/subchannel.cc | 25 +++++++++---------- .../ext/filters/client_channel/subchannel.h | 9 ++++--- test/cpp/microbenchmarks/bm_call_create.cc | 6 ++--- 5 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index 8d02304d19f..b66c920b909 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -172,9 +172,8 @@ grpc_json* SubchannelNode::RenderJson() { if (socket_uuid != 0) { grpc_json* array_parent = grpc_json_create_child( nullptr, json, "socketRef", nullptr, GRPC_JSON_ARRAY, false); - json_iterator = - grpc_json_create_child(json_iterator, array_parent, nullptr, nullptr, - GRPC_JSON_OBJECT, false); + json_iterator = grpc_json_create_child(json_iterator, array_parent, nullptr, + nullptr, GRPC_JSON_OBJECT, false); grpc_json_add_number_string_child(json_iterator, nullptr, "socketId", socket_uuid); } diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h index a6dae165223..8ce331e529d 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.h +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -76,9 +76,6 @@ class SubchannelNode : public BaseNode { grpc_json* RenderJson() override; - // helper to populate the socket(s) that this subchannel owns. - void PopulateChildSockets(grpc_json* json); - // proxy methods to composed classes. void AddTraceEvent(ChannelTrace::Severity severity, grpc_slice data) { trace_.AddTraceEvent(severity, data); diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 4c33636646c..4756733f1f8 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -97,10 +97,6 @@ struct grpc_subchannel { /** set during connection */ grpc_connect_out_args connecting_result; - /** uuid of this subchannel's socket. 0 if this subchannel is not - connected */ - intptr_t socket_uuid; - /** callback for connection finishing */ grpc_closure on_connected; @@ -258,7 +254,6 @@ static void disconnect(grpc_subchannel* c) { c->disconnected = true; grpc_connector_shutdown(c->connector, GRPC_ERROR_CREATE_FROM_STATIC_STRING( "Subchannel disconnected")); - c->socket_uuid = 0; c->connected_subchannel.reset(); gpr_mu_unlock(&c->mu); } @@ -416,9 +411,12 @@ grpc_core::channelz::SubchannelNode* grpc_subchannel_get_channelz_node( return subchannel->channelz_subchannel.get(); } -intptr_t grpc_subchannel_get_child_socket_uuid( - grpc_subchannel* subchannel) { - return subchannel->socket_uuid; +intptr_t grpc_subchannel_get_child_socket_uuid(grpc_subchannel* subchannel) { + if (subchannel->connected_subchannel != nullptr) { + return subchannel->connected_subchannel->socket_uuid(); + } else { + return 0; + } } static void continue_connect_locked(grpc_subchannel* c) { @@ -579,7 +577,6 @@ static void on_connected_subchannel_connectivity_changed(void* p, grpc_connectivity_state_name( connected_subchannel_watcher->connectivity_state)); } - c->socket_uuid = 0; c->connected_subchannel.reset(); grpc_connectivity_state_set(&c->state_tracker, GRPC_CHANNEL_TRANSIENT_FAILURE, @@ -632,7 +629,8 @@ static bool publish_transport_locked(grpc_subchannel* c) { GRPC_ERROR_UNREF(error); return false; } - c->socket_uuid = grpc_transport_get_socket_uuid(c->connecting_result.transport); + intptr_t socket_uuid = + grpc_transport_get_socket_uuid(c->connecting_result.transport); memset(&c->connecting_result, 0, sizeof(c->connecting_result)); /* initialize state watcher */ @@ -653,7 +651,7 @@ static bool publish_transport_locked(grpc_subchannel* c) { /* publish */ c->connected_subchannel.reset(grpc_core::New( - stk, c->channelz_subchannel.get())); + stk, c->channelz_subchannel.get(), socket_uuid)); gpr_log(GPR_INFO, "New connected subchannel at %p for subchannel %p", c->connected_subchannel.get(), c); @@ -823,10 +821,11 @@ namespace grpc_core { ConnectedSubchannel::ConnectedSubchannel( grpc_channel_stack* channel_stack, - channelz::SubchannelNode* channelz_subchannel) + channelz::SubchannelNode* channelz_subchannel, intptr_t socket_uuid) : RefCountedWithTracing(&grpc_trace_stream_refcount), channel_stack_(channel_stack), - channelz_subchannel_(channelz_subchannel) {} + channelz_subchannel_(channelz_subchannel), + socket_uuid_(socket_uuid) {} ConnectedSubchannel::~ConnectedSubchannel() { GRPC_CHANNEL_STACK_UNREF(channel_stack_, "connected_subchannel_dtor"); diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h index 67a2d497111..5d95a720853 100644 --- a/src/core/ext/filters/client_channel/subchannel.h +++ b/src/core/ext/filters/client_channel/subchannel.h @@ -86,7 +86,8 @@ class ConnectedSubchannel : public RefCountedWithTracing { }; explicit ConnectedSubchannel(grpc_channel_stack* channel_stack, - channelz::SubchannelNode* channelz_subchannel); + channelz::SubchannelNode* channelz_subchannel, + intptr_t socket_uuid); ~ConnectedSubchannel(); grpc_channel_stack* channel_stack() { return channel_stack_; } @@ -98,12 +99,15 @@ class ConnectedSubchannel : public RefCountedWithTracing { channelz::SubchannelNode* channelz_subchannel() { return channelz_subchannel_; } + intptr_t socket_uuid() { return socket_uuid_; } private: grpc_channel_stack* channel_stack_; // backpointer to the channelz node in this connected subchannel's // owning subchannel. channelz::SubchannelNode* channelz_subchannel_; + // uuid of this subchannel's socket. 0 if this subchannel is not connected. + intptr_t socket_uuid_; }; } // namespace grpc_core @@ -126,8 +130,7 @@ void grpc_subchannel_call_unref( grpc_core::channelz::SubchannelNode* grpc_subchannel_get_channelz_node( grpc_subchannel* subchannel); -intptr_t grpc_subchannel_get_child_socket_uuid( - grpc_subchannel* subchannel); +intptr_t grpc_subchannel_get_child_socket_uuid(grpc_subchannel* subchannel); /** Returns a pointer to the parent data associated with \a subchannel_call. The data will be of the size specified in \a parent_data_size diff --git a/test/cpp/microbenchmarks/bm_call_create.cc b/test/cpp/microbenchmarks/bm_call_create.cc index b0cccafcf82..45ac782a0e1 100644 --- a/test/cpp/microbenchmarks/bm_call_create.cc +++ b/test/cpp/microbenchmarks/bm_call_create.cc @@ -449,9 +449,9 @@ grpc_endpoint* GetEndpoint(grpc_transport* self) { return nullptr; } intptr_t GetSocketUuid(grpc_transport* t) { return 0; } static const grpc_transport_vtable dummy_transport_vtable = { - 0, "dummy_http2", InitStream, - SetPollset, SetPollsetSet, PerformStreamOp, - PerformOp, DestroyStream, Destroy, + 0, "dummy_http2", InitStream, + SetPollset, SetPollsetSet, PerformStreamOp, + PerformOp, DestroyStream, Destroy, GetEndpoint, GetSocketUuid}; static grpc_transport dummy_transport = {&dummy_transport_vtable}; From 404b2515af9c4dcc29440dea8b955ba341521b68 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 27 Sep 2018 11:18:42 -0500 Subject: [PATCH 4/4] reviewer feedback --- .../ext/filters/client_channel/connector.h | 3 +++ .../ext/filters/client_channel/subchannel.cc | 3 +-- .../ext/filters/client_channel/subchannel.h | 2 +- .../chttp2/client/chttp2_connector.cc | 2 ++ .../chttp2/transport/chttp2_transport.cc | 23 +++++++++---------- .../chttp2/transport/chttp2_transport.h | 2 ++ .../cronet/transport/cronet_transport.cc | 5 +--- .../ext/transport/inproc/inproc_transport.cc | 4 +--- src/core/lib/transport/transport.cc | 4 ---- src/core/lib/transport/transport.h | 2 -- src/core/lib/transport/transport_impl.h | 2 -- test/cpp/microbenchmarks/bm_call_create.cc | 10 ++++---- 12 files changed, 26 insertions(+), 36 deletions(-) diff --git a/src/core/ext/filters/client_channel/connector.h b/src/core/ext/filters/client_channel/connector.h index 556594929c1..ea34dcdab57 100644 --- a/src/core/ext/filters/client_channel/connector.h +++ b/src/core/ext/filters/client_channel/connector.h @@ -47,6 +47,9 @@ typedef struct { /** channel arguments (to be passed to the filters) */ grpc_channel_args* channel_args; + + /** socket uuid of the connected transport. 0 if not available */ + intptr_t socket_uuid; } grpc_connect_out_args; struct grpc_connector_vtable { diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 4756733f1f8..2847f4bdc18 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -629,8 +629,7 @@ static bool publish_transport_locked(grpc_subchannel* c) { GRPC_ERROR_UNREF(error); return false; } - intptr_t socket_uuid = - grpc_transport_get_socket_uuid(c->connecting_result.transport); + intptr_t socket_uuid = c->connecting_result.socket_uuid; memset(&c->connecting_result, 0, sizeof(c->connecting_result)); /* initialize state watcher */ diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h index 5d95a720853..699f93a8e77 100644 --- a/src/core/ext/filters/client_channel/subchannel.h +++ b/src/core/ext/filters/client_channel/subchannel.h @@ -107,7 +107,7 @@ class ConnectedSubchannel : public RefCountedWithTracing { // owning subchannel. channelz::SubchannelNode* channelz_subchannel_; // uuid of this subchannel's socket. 0 if this subchannel is not connected. - intptr_t socket_uuid_; + const intptr_t socket_uuid_; }; } // namespace grpc_core diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.cc b/src/core/ext/transport/chttp2/client/chttp2_connector.cc index e7522ffba87..0ac84032fd4 100644 --- a/src/core/ext/transport/chttp2/client/chttp2_connector.cc +++ b/src/core/ext/transport/chttp2/client/chttp2_connector.cc @@ -117,6 +117,8 @@ static void on_handshake_done(void* arg, grpc_error* error) { c->args.interested_parties); c->result->transport = grpc_create_chttp2_transport(args->args, args->endpoint, true); + c->result->socket_uuid = + grpc_chttp2_transport_get_socket_uuid(c->result->transport); GPR_ASSERT(c->result->transport); // TODO(roth): We ideally want to wait until we receive HTTP/2 // settings from the server before we consider the connection diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 45ef4161f78..776c15138b8 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -3157,16 +3157,6 @@ static grpc_endpoint* chttp2_get_endpoint(grpc_transport* t) { return (reinterpret_cast(t))->ep; } -static intptr_t get_socket_uuid(grpc_transport* transport) { - grpc_chttp2_transport* t = - reinterpret_cast(transport); - if (t->channelz_socket != nullptr) { - return t->channelz_socket->uuid(); - } else { - return 0; - } -} - static const grpc_transport_vtable vtable = {sizeof(grpc_chttp2_stream), "chttp2", init_stream, @@ -3176,11 +3166,20 @@ static const grpc_transport_vtable vtable = {sizeof(grpc_chttp2_stream), perform_transport_op, destroy_stream, destroy_transport, - chttp2_get_endpoint, - get_socket_uuid}; + chttp2_get_endpoint}; static const grpc_transport_vtable* get_vtable(void) { return &vtable; } +intptr_t grpc_chttp2_transport_get_socket_uuid(grpc_transport* transport) { + grpc_chttp2_transport* t = + reinterpret_cast(transport); + if (t->channelz_socket != nullptr) { + return t->channelz_socket->uuid(); + } else { + return 0; + } +} + grpc_transport* grpc_create_chttp2_transport( const grpc_channel_args* channel_args, grpc_endpoint* ep, bool is_client) { grpc_chttp2_transport* t = static_cast( diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.h b/src/core/ext/transport/chttp2/transport/chttp2_transport.h index 9d55b3f4b0d..e5872fee436 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.h +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.h @@ -34,6 +34,8 @@ extern bool g_flow_control_enabled; grpc_transport* grpc_create_chttp2_transport( const grpc_channel_args* channel_args, grpc_endpoint* ep, bool is_client); +intptr_t grpc_chttp2_transport_get_socket_uuid(grpc_transport* transport); + /// Takes ownership of \a read_buffer, which (if non-NULL) contains /// leftover bytes previously read from the endpoint (e.g., by handshakers). /// If non-null, \a notify_on_receive_settings will be scheduled when diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc index bb20d9db084..81e2634e3a7 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.cc +++ b/src/core/ext/transport/cronet/transport/cronet_transport.cc @@ -1439,8 +1439,6 @@ static grpc_endpoint* get_endpoint(grpc_transport* gt) { return nullptr; } static void perform_op(grpc_transport* gt, grpc_transport_op* op) {} -static intptr_t get_socket_uuid(grpc_transport* t) { return 0; } - static const grpc_transport_vtable grpc_cronet_vtable = { sizeof(stream_obj), "cronet_http", @@ -1451,8 +1449,7 @@ static const grpc_transport_vtable grpc_cronet_vtable = { perform_op, destroy_stream, destroy_transport, - get_endpoint, - get_socket_uuid}; + get_endpoint}; grpc_transport* grpc_create_cronet_transport(void* engine, const char* target, const grpc_channel_args* args, diff --git a/src/core/ext/transport/inproc/inproc_transport.cc b/src/core/ext/transport/inproc/inproc_transport.cc index 56951f83386..b0ca7f8207e 100644 --- a/src/core/ext/transport/inproc/inproc_transport.cc +++ b/src/core/ext/transport/inproc/inproc_transport.cc @@ -1170,8 +1170,6 @@ static void set_pollset_set(grpc_transport* gt, grpc_stream* gs, static grpc_endpoint* get_endpoint(grpc_transport* t) { return nullptr; } -static intptr_t get_socket_uuid(grpc_transport* t) { return 0; } - /******************************************************************************* * GLOBAL INIT AND DESTROY */ @@ -1196,7 +1194,7 @@ static const grpc_transport_vtable inproc_vtable = { sizeof(inproc_stream), "inproc", init_stream, set_pollset, set_pollset_set, perform_stream_op, perform_transport_op, destroy_stream, destroy_transport, - get_endpoint, get_socket_uuid}; + get_endpoint}; /******************************************************************************* * Main inproc transport functions diff --git a/src/core/lib/transport/transport.cc b/src/core/lib/transport/transport.cc index c8fdbb4de26..cbdb77c8441 100644 --- a/src/core/lib/transport/transport.cc +++ b/src/core/lib/transport/transport.cc @@ -199,10 +199,6 @@ grpc_endpoint* grpc_transport_get_endpoint(grpc_transport* transport) { return transport->vtable->get_endpoint(transport); } -intptr_t grpc_transport_get_socket_uuid(grpc_transport* transport) { - return transport->vtable->get_socket_uuid(transport); -} - // This comment should be sung to the tune of // "Supercalifragilisticexpialidocious": // diff --git a/src/core/lib/transport/transport.h b/src/core/lib/transport/transport.h index aad133f6c3d..9e784635c69 100644 --- a/src/core/lib/transport/transport.h +++ b/src/core/lib/transport/transport.h @@ -366,8 +366,6 @@ void grpc_transport_destroy(grpc_transport* transport); /* Get the endpoint used by \a transport */ grpc_endpoint* grpc_transport_get_endpoint(grpc_transport* transport); -intptr_t grpc_transport_get_socket_uuid(grpc_transport* transport); - /* Allocate a grpc_transport_op, and preconfigure the on_consumed closure to \a on_consumed and then delete the returned transport op */ grpc_transport_op* grpc_make_transport_op(grpc_closure* on_consumed); diff --git a/src/core/lib/transport/transport_impl.h b/src/core/lib/transport/transport_impl.h index d609470ef01..ba5e05df0af 100644 --- a/src/core/lib/transport/transport_impl.h +++ b/src/core/lib/transport/transport_impl.h @@ -60,8 +60,6 @@ typedef struct grpc_transport_vtable { /* implementation of grpc_transport_get_endpoint */ grpc_endpoint* (*get_endpoint)(grpc_transport* self); - - intptr_t (*get_socket_uuid)(grpc_transport* self); } grpc_transport_vtable; /* an instance of a grpc transport */ diff --git a/test/cpp/microbenchmarks/bm_call_create.cc b/test/cpp/microbenchmarks/bm_call_create.cc index 45ac782a0e1..9516b2e3e25 100644 --- a/test/cpp/microbenchmarks/bm_call_create.cc +++ b/test/cpp/microbenchmarks/bm_call_create.cc @@ -446,13 +446,11 @@ void Destroy(grpc_transport* self) {} /* implementation of grpc_transport_get_endpoint */ grpc_endpoint* GetEndpoint(grpc_transport* self) { return nullptr; } -intptr_t GetSocketUuid(grpc_transport* t) { return 0; } - static const grpc_transport_vtable dummy_transport_vtable = { - 0, "dummy_http2", InitStream, - SetPollset, SetPollsetSet, PerformStreamOp, - PerformOp, DestroyStream, Destroy, - GetEndpoint, GetSocketUuid}; + 0, "dummy_http2", InitStream, + SetPollset, SetPollsetSet, PerformStreamOp, + PerformOp, DestroyStream, Destroy, + GetEndpoint}; static grpc_transport dummy_transport = {&dummy_transport_vtable};