From 018498a06be9188bda95b5753e2aa93db2b5a28f Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Fri, 29 Jun 2018 14:48:05 -0700
Subject: [PATCH 01/14] Implements subchannel refs for pick_first
---
grpc.def | 1 +
include/grpc/support/string_util.h | 4 ++
.../filters/client_channel/client_channel.cc | 10 +++
.../filters/client_channel/client_channel.h | 6 ++
.../client_channel/client_channel_channelz.cc | 31 ++++++++
.../client_channel/client_channel_channelz.h | 8 +++
.../ext/filters/client_channel/lb_policy.cc | 17 ++++-
.../ext/filters/client_channel/lb_policy.h | 17 +++++
.../lb_policy/pick_first/pick_first.cc | 51 +++++++++++++
.../ext/filters/client_channel/subchannel.cc | 12 ++++
.../ext/filters/client_channel/subchannel.h | 2 +
src/core/lib/channel/channel_trace.cc | 44 ++----------
src/core/lib/channel/channelz.cc | 71 +++++--------------
src/core/lib/channel/channelz.h | 2 +
src/core/lib/gpr/string.cc | 34 +++++++++
src/core/lib/iomgr/error.cc | 3 +-
src/core/lib/json/json.cc | 10 +++
src/core/lib/json/json.h | 4 ++
src/ruby/ext/grpc/rb_grpc_imports.generated.c | 2 +
src/ruby/ext/grpc/rb_grpc_imports.generated.h | 3 +
test/core/end2end/tests/simple_request.cc | 32 ++++-----
.../core/surface/public_headers_must_be_c89.c | 1 +
22 files changed, 254 insertions(+), 111 deletions(-)
diff --git a/grpc.def b/grpc.def
index 06db74cad58..7d33e085754 100644
--- a/grpc.def
+++ b/grpc.def
@@ -199,6 +199,7 @@ EXPORTS
gpr_format_message
gpr_strdup
gpr_asprintf
+ gpr_format_timespec
gpr_mu_init
gpr_mu_destroy
gpr_mu_lock
diff --git a/include/grpc/support/string_util.h b/include/grpc/support/string_util.h
index 2c7460fa157..3b5b0008b24 100644
--- a/include/grpc/support/string_util.h
+++ b/include/grpc/support/string_util.h
@@ -21,6 +21,8 @@
#include <grpc/support/port_platform.h>
+#include <grpc/impl/codegen/gpr_types.h>
+
#ifdef __cplusplus
extern "C" {
#endif
@@ -42,6 +44,8 @@ GPRAPI char* gpr_strdup(const char* src);
GPRAPI int gpr_asprintf(char** strp, const char* format, ...)
GPR_PRINT_FORMAT_CHECK(2, 3);
+GPRAPI char* gpr_format_timespec(gpr_timespec);
+
#ifdef __cplusplus
}
#endif
diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc
index 520431e63b6..8c3e9f2b305 100644
--- a/src/core/ext/filters/client_channel/client_channel.cc
+++ b/src/core/ext/filters/client_channel/client_channel.cc
@@ -3159,6 +3159,16 @@ static void try_to_connect_locked(void* arg, grpc_error* error_ignored) {
GRPC_CHANNEL_STACK_UNREF(chand->owning_stack, "try_to_connect");
}
+void grpc_client_channel_populate_child_refs(
+ grpc_channel_element* elem, grpc_core::ChildRefsList* child_subchannels,
+ grpc_core::ChildRefsList* child_channels) {
+ channel_data* chand = static_cast<channel_data*>(elem->channel_data);
+ if (chand->lb_policy) {
+ chand->lb_policy->FillChildRefsForChannelz(child_subchannels,
+ child_channels);
+ }
+}
+
grpc_connectivity_state grpc_client_channel_check_connectivity_state(
grpc_channel_element* elem, int try_to_connect) {
channel_data* chand = static_cast<channel_data*>(elem->channel_data);
diff --git a/src/core/ext/filters/client_channel/client_channel.h b/src/core/ext/filters/client_channel/client_channel.h
index a21e5623a7b..0f95f6670a6 100644
--- a/src/core/ext/filters/client_channel/client_channel.h
+++ b/src/core/ext/filters/client_channel/client_channel.h
@@ -21,9 +21,11 @@
#include <grpc/support/port_platform.h>
+#include "src/core/ext/filters/client_channel/client_channel_channelz.h"
#include "src/core/ext/filters/client_channel/client_channel_factory.h"
#include "src/core/ext/filters/client_channel/resolver.h"
#include "src/core/lib/channel/channel_stack.h"
+#include "src/core/lib/json/json.h"
extern grpc_core::TraceFlag grpc_client_channel_trace;
@@ -39,6 +41,10 @@ extern grpc_core::TraceFlag grpc_client_channel_trace;
extern const grpc_channel_filter grpc_client_channel_filter;
+void grpc_client_channel_populate_child_refs(
+ grpc_channel_element* elem, grpc_core::ChildRefsList* child_subchannels,
+ grpc_core::ChildRefsList* child_channels);
+
grpc_connectivity_state grpc_client_channel_check_connectivity_state(
grpc_channel_element* elem, int try_to_connect);
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 08ceb2dd05a..235b8f3207c 100644
--- a/src/core/ext/filters/client_channel/client_channel_channelz.cc
+++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc
@@ -63,6 +63,37 @@ void ClientChannelNode::PopulateConnectivityState(grpc_json* json) {
false);
}
+void ClientChannelNode::PopulateChildRefs(grpc_json* json) {
+ ChildRefsList child_subchannels;
+ ChildRefsList child_channels;
+ grpc_json* json_iterator = nullptr;
+ grpc_client_channel_populate_child_refs(client_channel_, &child_subchannels,
+ &child_channels);
+ if (child_subchannels.size() > 0) {
+ grpc_json* array_parent = grpc_json_create_child(
+ nullptr, json, "subchannelRef", nullptr, GRPC_JSON_ARRAY, false);
+ for (size_t i = 0; i < child_subchannels.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, "subchannelId",
+ child_subchannels[i]);
+ }
+ }
+ if (child_channels.size() > 0) {
+ grpc_json* array_parent = grpc_json_create_child(
+ nullptr, json, "channelRef", nullptr, GRPC_JSON_ARRAY, false);
+ json_iterator = nullptr;
+ for (size_t i = 0; i < child_subchannels.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, "channelId",
+ child_subchannels[i]);
+ }
+ }
+}
+
grpc_arg ClientChannelNode::CreateChannelArg() {
return grpc_channel_arg_pointer_create(
const_cast<char*>(GRPC_ARG_CHANNELZ_CHANNEL_NODE_CREATION_FUNC),
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 cf3ef7b6f28..e5be52e7785 100644
--- a/src/core/ext/filters/client_channel/client_channel_channelz.h
+++ b/src/core/ext/filters/client_channel/client_channel_channelz.h
@@ -22,9 +22,14 @@
#include <grpc/support/port_platform.h>
#include "src/core/lib/channel/channel_args.h"
+#include "src/core/lib/channel/channel_stack.h"
#include "src/core/lib/channel/channelz.h"
+#include "src/core/lib/gprpp/inlined_vector.h"
namespace grpc_core {
+
+typedef InlinedVector<intptr_t, 10> ChildRefsList;
+
namespace channelz {
// Subtype of ChannelNode that overrides and provides client_channel specific
@@ -38,6 +43,9 @@ class ClientChannelNode : public ChannelNode {
// channel connectivity.
void PopulateConnectivityState(grpc_json* json) override;
+ // Override this functionality since client_channels have subchannels
+ void PopulateChildRefs(grpc_json* json) override;
+
// Helper to create a channel arg to ensure this type of ChannelNode is
// created.
static grpc_arg CreateChannelArg();
diff --git a/src/core/ext/filters/client_channel/lb_policy.cc b/src/core/ext/filters/client_channel/lb_policy.cc
index e065f45639b..a0d7758f235 100644
--- a/src/core/ext/filters/client_channel/lb_policy.cc
+++ b/src/core/ext/filters/client_channel/lb_policy.cc
@@ -31,13 +31,28 @@ LoadBalancingPolicy::LoadBalancingPolicy(const Args& args)
combiner_(GRPC_COMBINER_REF(args.combiner, "lb_policy")),
client_channel_factory_(args.client_channel_factory),
interested_parties_(grpc_pollset_set_create()),
- request_reresolution_(nullptr) {}
+ request_reresolution_(nullptr) {
+ gpr_mu_init(&child_refs_mu_);
+}
LoadBalancingPolicy::~LoadBalancingPolicy() {
grpc_pollset_set_destroy(interested_parties_);
+ gpr_mu_destroy(&child_refs_mu_);
GRPC_COMBINER_UNREF(combiner_, "lb_policy");
}
+void LoadBalancingPolicy::FillChildRefsForChannelz(
+ ChildRefsList* child_subchannels, ChildRefsList* child_channels) {
+ mu_guard guard(&child_refs_mu_);
+ // TODO, de dup these.
+ for (size_t i = 0; i < child_subchannels_.size(); ++i) {
+ child_subchannels->push_back(child_subchannels_[i]);
+ }
+ for (size_t i = 0; i < child_channels_.size(); ++i) {
+ child_channels->push_back(child_channels_[i]);
+ }
+}
+
void LoadBalancingPolicy::TryReresolutionLocked(
grpc_core::TraceFlag* grpc_lb_trace, grpc_error* error) {
if (request_reresolution_ != nullptr) {
diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h
index dab4466b21a..0c5fd2ad9cf 100644
--- a/src/core/ext/filters/client_channel/lb_policy.h
+++ b/src/core/ext/filters/client_channel/lb_policy.h
@@ -21,6 +21,7 @@
#include <grpc/support/port_platform.h>
+#include "src/core/ext/filters/client_channel/client_channel_channelz.h"
#include "src/core/ext/filters/client_channel/client_channel_factory.h"
#include "src/core/ext/filters/client_channel/subchannel.h"
#include "src/core/lib/gprpp/abstract.h"
@@ -28,6 +29,7 @@
#include "src/core/lib/gprpp/ref_counted_ptr.h"
#include "src/core/lib/iomgr/combiner.h"
#include "src/core/lib/iomgr/polling_entity.h"
+#include "src/core/lib/json/json.h"
#include "src/core/lib/transport/connectivity_state.h"
extern grpc_core::DebugOnlyTraceFlag grpc_trace_lb_policy_refcount;
@@ -157,6 +159,13 @@ class LoadBalancingPolicy
request_reresolution_ = request_reresolution;
}
+ /// populates child_subchannels and child_channels with the uuids of this
+ /// LB policies referenced children. This is not invoked from the
+ /// client_channel's combiner. It has its own synchronization. This is
+ /// not abstract, since the behavior is the same for all LB policies.
+ void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
+ ChildRefsList* child_channels);
+
grpc_pollset_set* interested_parties() const { return interested_parties_; }
GRPC_ABSTRACT_BASE_CLASS
@@ -171,6 +180,9 @@ class LoadBalancingPolicy
grpc_client_channel_factory* client_channel_factory() const {
return client_channel_factory_;
}
+ gpr_mu* child_refs_mu() { return &child_refs_mu_; }
+ ChildRefsList* child_subchannels() { return &child_subchannels_; }
+ ChildRefsList* child_channels() { return &child_channels_; }
/// Shuts down the policy. Any pending picks that have not been
/// handed off to a new policy via HandOffPendingPicksLocked() will be
@@ -190,6 +202,11 @@ class LoadBalancingPolicy
/// Combiner under which LB policy actions take place.
grpc_combiner* combiner_;
+ /// Lock and data used to capture snapshots of this channels child
+ /// channels and subchannels. This data is consumed by channelz.
+ gpr_mu child_refs_mu_;
+ ChildRefsList child_subchannels_;
+ ChildRefsList child_channels_;
/// Client channel factory, used to create channels and subchannels.
grpc_client_channel_factory* client_channel_factory_;
/// Owned pointer to interested parties in load balancing decisions.
diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index ff2140e628a..e43d1c2ab46 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -37,6 +37,15 @@ TraceFlag grpc_lb_pick_first_trace(false, "pick_first");
namespace {
+class LockGuard {
+ public:
+ LockGuard(gpr_mu* mu) : mu_(mu) { gpr_mu_lock(mu_); }
+ ~LockGuard() { gpr_mu_unlock(mu_); }
+
+ private:
+ gpr_mu* mu_;
+};
+
//
// pick_first LB policy
//
@@ -103,10 +112,20 @@ class PickFirst : public LoadBalancingPolicy {
}
};
+ class UpdateGuard {
+ public:
+ UpdateGuard(PickFirst* pf) : pf_(pf) {}
+ ~UpdateGuard() { pf_->UpdateChildRefsLocked(); }
+
+ private:
+ PickFirst* pf_;
+ };
+
void ShutdownLocked() override;
void StartPickingLocked();
void DestroyUnselectedSubchannelsLocked();
+ void UpdateChildRefsLocked();
// All our subchannels.
OrphanablePtr<PickFirstSubchannelList> subchannel_list_;
@@ -158,6 +177,7 @@ void PickFirst::HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) {
}
void PickFirst::ShutdownLocked() {
+ UpdateGuard(this);
grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Channel shutdown");
if (grpc_lb_pick_first_trace.enabled()) {
gpr_log(GPR_INFO, "Pick First %p Shutting down", this);
@@ -280,7 +300,37 @@ void PickFirst::PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) {
}
}
+void PickFirst::UpdateChildRefsLocked() {
+ mu_guard guard(child_refs_mu());
+ // reset both lists
+ child_subchannels()->clear();
+ // this will stay empty, because pick_first channels have no children
+ // channels.
+ child_channels()->clear();
+ // populate the subchannels with boths subchannels lists, they will be
+ // deduped when the actual channelz query comes in.
+ if (subchannel_list_ != nullptr) {
+ for (size_t i = 0; i < subchannel_list_->num_subchannels(); ++i) {
+ if (subchannel_list_->subchannel(i)->subchannel() != nullptr) {
+ child_subchannels()->push_back(grpc_subchannel_get_uuid(
+ subchannel_list_->subchannel(i)->subchannel()));
+ }
+ }
+ }
+ if (latest_pending_subchannel_list_ != nullptr) {
+ for (size_t i = 0; i < latest_pending_subchannel_list_->num_subchannels();
+ ++i) {
+ if (latest_pending_subchannel_list_->subchannel(i)->subchannel() !=
+ nullptr) {
+ child_subchannels()->push_back(grpc_subchannel_get_uuid(
+ latest_pending_subchannel_list_->subchannel(i)->subchannel()));
+ }
+ }
+ }
+}
+
void PickFirst::UpdateLocked(const grpc_channel_args& args) {
+ UpdateGuard guard(this);
const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES);
if (arg == nullptr || arg->type != GRPC_ARG_POINTER) {
if (subchannel_list_ == nullptr) {
@@ -388,6 +438,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) {
void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked(
grpc_connectivity_state connectivity_state, grpc_error* error) {
PickFirst* p = static_cast<PickFirst*>(subchannel_list()->policy());
+ UpdateGuard guard(p);
// The notification must be for a subchannel in either the current or
// latest pending subchannel lists.
GPR_ASSERT(subchannel_list() == p->subchannel_list_.get() ||
diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc
index 8ab3fe40f50..6c4f1869eae 100644
--- a/src/core/ext/filters/client_channel/subchannel.cc
+++ b/src/core/ext/filters/client_channel/subchannel.cc
@@ -134,6 +134,11 @@ struct grpc_subchannel {
bool backoff_begun;
/** our alarm */
grpc_timer alarm;
+
+ /* the global uuid for this subchannel */
+ // TODO(ncteisen): move this into SubchannelNode while implementing
+ // GetSubchannel.
+ intptr_t subchannel_uuid;
};
struct grpc_subchannel_call {
@@ -374,9 +379,16 @@ grpc_subchannel* grpc_subchannel_create(grpc_connector* connector,
c->backoff.Init(backoff_options);
gpr_mu_init(&c->mu);
+ // This is just a placeholder for now
+ c->subchannel_uuid = 42;
+
return grpc_subchannel_index_register(key, c);
}
+intptr_t grpc_subchannel_get_uuid(grpc_subchannel* s) {
+ return s->subchannel_uuid;
+}
+
static void continue_connect_locked(grpc_subchannel* c) {
grpc_connect_in_args args;
args.interested_parties = c->pollset_set;
diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h
index e23aec12df4..590e80f507e 100644
--- a/src/core/ext/filters/client_channel/subchannel.h
+++ b/src/core/ext/filters/client_channel/subchannel.h
@@ -115,6 +115,8 @@ grpc_subchannel_call* grpc_subchannel_call_ref(
void grpc_subchannel_call_unref(
grpc_subchannel_call* call GRPC_SUBCHANNEL_REF_EXTRA_ARGS);
+intptr_t grpc_subchannel_get_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
field of the args passed to \a grpc_connected_subchannel_create_call(). */
diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc
index 0f655d87160..c17885a7134 100644
--- a/src/core/lib/channel/channel_trace.cc
+++ b/src/core/lib/channel/channel_trace.cc
@@ -131,38 +131,6 @@ void ChannelTrace::AddTraceEventReferencingSubchannel(
namespace {
-// returns an allocated string that represents tm according to RFC-3339, and,
-// more specifically, follows:
-// https://developers.google.com/protocol-buffers/docs/proto3#json
-//
-// "Uses RFC 3339, where generated output will always be Z-normalized and uses
-// 0, 3, 6 or 9 fractional digits."
-char* fmt_time(gpr_timespec tm) {
- char time_buffer[35];
- char ns_buffer[11]; // '.' + 9 digits of precision
- struct tm* tm_info = localtime((const time_t*)&tm.tv_sec);
- strftime(time_buffer, sizeof(time_buffer), "%Y-%m-%dT%H:%M:%S", tm_info);
- snprintf(ns_buffer, 11, ".%09d", tm.tv_nsec);
- // This loop trims off trailing zeros by inserting a null character that the
- // right point. We iterate in chunks of three because we want 0, 3, 6, or 9
- // fractional digits.
- for (int i = 7; i >= 1; i -= 3) {
- if (ns_buffer[i] == '0' && ns_buffer[i + 1] == '0' &&
- ns_buffer[i + 2] == '0') {
- ns_buffer[i] = '\0';
- // Edge case in which all fractional digits were 0.
- if (i == 1) {
- ns_buffer[0] = '\0';
- }
- } else {
- break;
- }
- }
- char* full_time_str;
- gpr_asprintf(&full_time_str, "%s%sZ", time_buffer, ns_buffer);
- return full_time_str;
-}
-
const char* severity_string(ChannelTrace::Severity severity) {
switch (severity) {
case ChannelTrace::Severity::Info:
@@ -186,9 +154,9 @@ void ChannelTrace::TraceEvent::RenderTraceEvent(grpc_json* json) const {
json_iterator = grpc_json_create_child(json_iterator, json, "severity",
severity_string(severity_),
GRPC_JSON_STRING, false);
- json_iterator =
- grpc_json_create_child(json_iterator, json, "timestamp",
- fmt_time(timestamp_), GRPC_JSON_STRING, true);
+ json_iterator = grpc_json_create_child(json_iterator, json, "timestamp",
+ gpr_format_timespec(timestamp_),
+ GRPC_JSON_STRING, true);
if (referenced_channel_ != nullptr) {
char* uuid_str;
gpr_asprintf(&uuid_str, "%" PRIdPTR, referenced_channel_->channel_uuid());
@@ -216,9 +184,9 @@ grpc_json* ChannelTrace::RenderJSON() const {
json_iterator =
grpc_json_create_child(json_iterator, json, "numEventsLogged",
num_events_logged_str, GRPC_JSON_STRING, true);
- json_iterator =
- grpc_json_create_child(json_iterator, json, "creationTimestamp",
- fmt_time(time_created_), GRPC_JSON_STRING, true);
+ json_iterator = grpc_json_create_child(
+ json_iterator, json, "creationTimestamp",
+ gpr_format_timespec(time_created_), GRPC_JSON_STRING, true);
grpc_json* events = grpc_json_create_child(json_iterator, json, "events",
nullptr, GRPC_JSON_ARRAY, false);
json_iterator = nullptr;
diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc
index 2074cb0cc53..ffc7ff884cd 100644
--- a/src/core/lib/channel/channelz.cc
+++ b/src/core/lib/channel/channelz.cc
@@ -41,53 +41,6 @@
namespace grpc_core {
namespace channelz {
-namespace {
-
-// TODO(ncteisen): move this function to a common helper location.
-//
-// returns an allocated string that represents tm according to RFC-3339, and,
-// more specifically, follows:
-// https://developers.google.com/protocol-buffers/docs/proto3#json
-//
-// "Uses RFC 3339, where generated output will always be Z-normalized and uses
-// 0, 3, 6 or 9 fractional digits."
-char* fmt_time(gpr_timespec tm) {
- char time_buffer[35];
- char ns_buffer[11]; // '.' + 9 digits of precision
- struct tm* tm_info = localtime((const time_t*)&tm.tv_sec);
- strftime(time_buffer, sizeof(time_buffer), "%Y-%m-%dT%H:%M:%S", tm_info);
- snprintf(ns_buffer, 11, ".%09d", tm.tv_nsec);
- // This loop trims off trailing zeros by inserting a null character that the
- // right point. We iterate in chunks of three because we want 0, 3, 6, or 9
- // fractional digits.
- for (int i = 7; i >= 1; i -= 3) {
- if (ns_buffer[i] == '0' && ns_buffer[i + 1] == '0' &&
- ns_buffer[i + 2] == '0') {
- ns_buffer[i] = '\0';
- // Edge case in which all fractional digits were 0.
- if (i == 1) {
- ns_buffer[0] = '\0';
- }
- } else {
- break;
- }
- }
- char* full_time_str;
- gpr_asprintf(&full_time_str, "%s%sZ", time_buffer, ns_buffer);
- return full_time_str;
-}
-
-// TODO(ncteisen); move this to json library
-grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name,
- int64_t num) {
- char* num_str;
- gpr_asprintf(&num_str, "%" PRId64, num);
- return grpc_json_create_child(it, parent, name, num_str, GRPC_JSON_STRING,
- true);
-}
-
-} // namespace
-
ChannelNode::ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes)
: channel_(channel), target_(nullptr), channel_uuid_(-1) {
trace_.Init(channel_tracer_max_nodes);
@@ -110,6 +63,8 @@ void ChannelNode::RecordCallStarted() {
void ChannelNode::PopulateConnectivityState(grpc_json* json) {}
+void ChannelNode::PopulateChildRefs(grpc_json* json) {}
+
char* ChannelNode::RenderJSON() {
// We need to track these three json objects to build our object
grpc_json* top_level_json = grpc_json_create(GRPC_JSON_OBJECT);
@@ -120,7 +75,8 @@ char* ChannelNode::RenderJSON() {
GRPC_JSON_OBJECT, false);
json = json_iterator;
json_iterator = nullptr;
- json_iterator = add_num_str(json, json_iterator, "channelId", channel_uuid_);
+ json_iterator = grpc_json_add_number_string_child(json, json_iterator,
+ "channelId", channel_uuid_);
// reset json iterators to top level object
json = top_level_json;
json_iterator = nullptr;
@@ -148,17 +104,22 @@ char* ChannelNode::RenderJSON() {
json_iterator = nullptr;
// We use -1 as sentinel values since proto default value for integers is
// zero, and the confuses the parser into thinking the value weren't present
- json_iterator =
- add_num_str(json, json_iterator, "callsStarted", calls_started_);
- json_iterator =
- add_num_str(json, json_iterator, "callsSucceeded", calls_succeeded_);
- json_iterator =
- add_num_str(json, json_iterator, "callsFailed", calls_failed_);
+ json_iterator = grpc_json_add_number_string_child(
+ json, json_iterator, "callsStarted", calls_started_);
+ json_iterator = grpc_json_add_number_string_child(
+ json, json_iterator, "callsSucceeded", calls_succeeded_);
+ json_iterator = grpc_json_add_number_string_child(
+ json, json_iterator, "callsFailed", calls_failed_);
gpr_timespec ts =
grpc_millis_to_timespec(last_call_started_millis_, GPR_CLOCK_REALTIME);
json_iterator =
grpc_json_create_child(json_iterator, json, "lastCallStartedTimestamp",
- fmt_time(ts), GRPC_JSON_STRING, true);
+ gpr_format_timespec(ts), GRPC_JSON_STRING, true);
+
+ json = top_level_json;
+ json_iterator = nullptr;
+ PopulateChildRefs(json);
+
// render and return the over json object
char* json_str = grpc_json_dump_to_string(top_level_json, 0);
grpc_json_destroy(top_level_json);
diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h
index 9bd01ece502..e84c187f11a 100644
--- a/src/core/lib/channel/channelz.h
+++ b/src/core/lib/channel/channelz.h
@@ -62,6 +62,8 @@ class ChannelNode : public RefCounted<ChannelNode> {
// instead of lib/
virtual void PopulateConnectivityState(grpc_json* json);
+ virtual void PopulateChildRefs(grpc_json* json);
+
ChannelTrace* trace() { return trace_.get(); }
void MarkChannelDestroyed() {
diff --git a/src/core/lib/gpr/string.cc b/src/core/lib/gpr/string.cc
index ef2a6900b45..9bcb6714a39 100644
--- a/src/core/lib/gpr/string.cc
+++ b/src/core/lib/gpr/string.cc
@@ -23,8 +23,10 @@
#include <ctype.h>
#include <limits.h>
#include <stddef.h>
+#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <time.h>
#include <grpc/support/alloc.h>
#include <grpc/support/log.h>
@@ -54,6 +56,38 @@ typedef struct {
char* data;
} dump_out;
+// Returns an allocated string that represents tm according to RFC-3339, and,
+// more specifically, follows:
+// https://developers.google.com/protocol-buffers/docs/proto3#json
+//
+// "Uses RFC 3339, where generated output will always be Z-normalized and uses
+// 0, 3, 6 or 9 fractional digits."
+char* gpr_format_timespec(gpr_timespec tm) {
+ char time_buffer[35];
+ char ns_buffer[11]; // '.' + 9 digits of precision
+ struct tm* tm_info = localtime((const time_t*)&tm.tv_sec);
+ strftime(time_buffer, sizeof(time_buffer), "%Y-%m-%dT%H:%M:%S", tm_info);
+ snprintf(ns_buffer, 11, ".%09d", tm.tv_nsec);
+ // This loop trims off trailing zeros by inserting a null character that the
+ // right point. We iterate in chunks of three because we want 0, 3, 6, or 9
+ // fractional digits.
+ for (int i = 7; i >= 1; i -= 3) {
+ if (ns_buffer[i] == '0' && ns_buffer[i + 1] == '0' &&
+ ns_buffer[i + 2] == '0') {
+ ns_buffer[i] = '\0';
+ // Edge case in which all fractional digits were 0.
+ if (i == 1) {
+ ns_buffer[0] = '\0';
+ }
+ } else {
+ break;
+ }
+ }
+ char* full_time_str;
+ gpr_asprintf(&full_time_str, "%s%sZ", time_buffer, ns_buffer);
+ return full_time_str;
+}
+
static dump_out dump_out_create(void) {
dump_out r = {0, 0, nullptr};
return r;
diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc
index 90ed34da11f..8e7205f5586 100644
--- a/src/core/lib/iomgr/error.cc
+++ b/src/core/lib/iomgr/error.cc
@@ -670,7 +670,8 @@ static void collect_times_kvs(grpc_error* err, kv_pairs* kvs) {
uint8_t slot = err->times[which];
if (slot != UINT8_MAX) {
append_kv(kvs, key_time(static_cast<grpc_error_times>(which)),
- fmt_time(*reinterpret_cast<gpr_timespec*>(err->arena + slot)));
+ gpr_format_timespec(
+ *reinterpret_cast<gpr_timespec*>(err->arena + slot)));
}
}
}
diff --git a/src/core/lib/json/json.cc b/src/core/lib/json/json.cc
index 816241bbf0e..6d359573f86 100644
--- a/src/core/lib/json/json.cc
+++ b/src/core/lib/json/json.cc
@@ -18,10 +18,12 @@
#include <grpc/support/port_platform.h>
+#include <inttypes.h>
#include <string.h>
#include <grpc/support/alloc.h>
#include <grpc/support/log.h>
+#include <grpc/support/string_util.h>
#include "src/core/lib/json/json.h"
@@ -84,3 +86,11 @@ grpc_json* grpc_json_create_child(grpc_json* sibling, grpc_json* parent,
child->key = key;
return child;
}
+
+grpc_json* grpc_json_add_number_string_child(grpc_json* parent, grpc_json* it,
+ const char* name, int64_t num) {
+ char* num_str;
+ gpr_asprintf(&num_str, "%" PRId64, num);
+ return grpc_json_create_child(it, parent, name, num_str, GRPC_JSON_STRING,
+ true);
+}
diff --git a/src/core/lib/json/json.h b/src/core/lib/json/json.h
index f93b43048b9..8742774b5fc 100644
--- a/src/core/lib/json/json.h
+++ b/src/core/lib/json/json.h
@@ -91,4 +91,8 @@ grpc_json* grpc_json_create_child(grpc_json* sibling, grpc_json* parent,
const char* key, const char* value,
grpc_json_type type, bool owns_value);
+/* TODO */
+grpc_json* grpc_json_add_number_string_child(grpc_json* parent, grpc_json* it,
+ const char* name, int64_t num);
+
#endif /* GRPC_CORE_LIB_JSON_JSON_H */
diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c
index 4e235121e24..1086b7dfcb9 100644
--- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c
+++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c
@@ -222,6 +222,7 @@ gpr_cpu_current_cpu_type gpr_cpu_current_cpu_import;
gpr_format_message_type gpr_format_message_import;
gpr_strdup_type gpr_strdup_import;
gpr_asprintf_type gpr_asprintf_import;
+gpr_format_timespec_type gpr_format_timespec_import;
gpr_mu_init_type gpr_mu_init_import;
gpr_mu_destroy_type gpr_mu_destroy_import;
gpr_mu_lock_type gpr_mu_lock_import;
@@ -470,6 +471,7 @@ void grpc_rb_load_imports(HMODULE library) {
gpr_format_message_import = (gpr_format_message_type) GetProcAddress(library, "gpr_format_message");
gpr_strdup_import = (gpr_strdup_type) GetProcAddress(library, "gpr_strdup");
gpr_asprintf_import = (gpr_asprintf_type) GetProcAddress(library, "gpr_asprintf");
+ gpr_format_timespec_import = (gpr_format_timespec_type) GetProcAddress(library, "gpr_format_timespec");
gpr_mu_init_import = (gpr_mu_init_type) GetProcAddress(library, "gpr_mu_init");
gpr_mu_destroy_import = (gpr_mu_destroy_type) GetProcAddress(library, "gpr_mu_destroy");
gpr_mu_lock_import = (gpr_mu_lock_type) GetProcAddress(library, "gpr_mu_lock");
diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h
index f01c9c82482..b4107641a48 100644
--- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h
+++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h
@@ -641,6 +641,9 @@ extern gpr_strdup_type gpr_strdup_import;
typedef int(*gpr_asprintf_type)(char** strp, const char* format, ...) GPR_PRINT_FORMAT_CHECK(2, 3);
extern gpr_asprintf_type gpr_asprintf_import;
#define gpr_asprintf gpr_asprintf_import
+typedef char*(*gpr_format_timespec_type)(gpr_timespec);
+extern gpr_format_timespec_type gpr_format_timespec_import;
+#define gpr_format_timespec gpr_format_timespec_import
typedef void(*gpr_mu_init_type)(gpr_mu* mu);
extern gpr_mu_init_type gpr_mu_init_import;
#define gpr_mu_init gpr_mu_init_import
diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc
index 941d9ae3198..7ec11d8ac3d 100644
--- a/test/core/end2end/tests/simple_request.cc
+++ b/test/core/end2end/tests/simple_request.cc
@@ -256,24 +256,24 @@ static void test_invoke_simple_request(grpc_end2end_test_config config) {
config.tear_down_data(&f);
}
-static void test_invoke_10_simple_requests(grpc_end2end_test_config config) {
- int i;
- grpc_end2end_test_fixture f =
- begin_test(config, "test_invoke_10_simple_requests", nullptr, nullptr);
- for (i = 0; i < 10; i++) {
- simple_request_body(config, f);
- gpr_log(GPR_INFO, "Running test: Passed simple request %d", i);
- }
- end_test(&f);
- config.tear_down_data(&f);
-}
+// static void test_invoke_10_simple_requests(grpc_end2end_test_config config) {
+// int i;
+// grpc_end2end_test_fixture f =
+// begin_test(config, "test_invoke_10_simple_requests", nullptr, nullptr);
+// for (i = 0; i < 10; i++) {
+// simple_request_body(config, f);
+// gpr_log(GPR_INFO, "Running test: Passed simple request %d", i);
+// }
+// end_test(&f);
+// config.tear_down_data(&f);
+// }
void simple_request(grpc_end2end_test_config config) {
- int i;
- for (i = 0; i < 10; i++) {
- test_invoke_simple_request(config);
- }
- test_invoke_10_simple_requests(config);
+ // int i;
+ // for (i = 0; i < 10; i++) {
+ test_invoke_simple_request(config);
+ // }
+ // test_invoke_10_simple_requests(config);
}
void simple_request_pre_init(void) {}
diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c
index 9a79b468dd5..95deb1f17a3 100644
--- a/test/core/surface/public_headers_must_be_c89.c
+++ b/test/core/surface/public_headers_must_be_c89.c
@@ -257,6 +257,7 @@ int main(int argc, char **argv) {
printf("%lx", (unsigned long) gpr_cpu_current_cpu);
printf("%lx", (unsigned long) gpr_strdup);
printf("%lx", (unsigned long) gpr_asprintf);
+ printf("%lx", (unsigned long) gpr_format_timespec);
printf("%lx", (unsigned long) gpr_mu_init);
printf("%lx", (unsigned long) gpr_mu_destroy);
printf("%lx", (unsigned long) gpr_mu_lock);
From 25082c5fc47d0b337657476a0fa7ce989b9712ef Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Thu, 12 Jul 2018 17:24:48 -0700
Subject: [PATCH 02/14] Reviewer feedback
---
include/grpc/support/string_util.h | 2 --
.../filters/client_channel/client_channel.cc | 2 +-
.../filters/client_channel/client_channel.h | 1 -
.../ext/filters/client_channel/lb_policy.cc | 12 ----------
.../ext/filters/client_channel/lb_policy.h | 16 ++++++-------
.../lb_policy/pick_first/pick_first.cc | 24 +++++++------------
src/core/lib/channel/channelz.cc | 1 -
src/core/lib/gpr/string.cc | 6 -----
src/core/lib/gpr/string.h | 10 ++++++++
src/core/lib/gprpp/abstract.h | 7 ++++--
src/core/lib/iomgr/error.cc | 3 +--
src/core/lib/json/json.h | 3 ++-
12 files changed, 36 insertions(+), 51 deletions(-)
diff --git a/include/grpc/support/string_util.h b/include/grpc/support/string_util.h
index 3b5b0008b24..2679160c1bb 100644
--- a/include/grpc/support/string_util.h
+++ b/include/grpc/support/string_util.h
@@ -44,8 +44,6 @@ GPRAPI char* gpr_strdup(const char* src);
GPRAPI int gpr_asprintf(char** strp, const char* format, ...)
GPR_PRINT_FORMAT_CHECK(2, 3);
-GPRAPI char* gpr_format_timespec(gpr_timespec);
-
#ifdef __cplusplus
}
#endif
diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc
index 8c3e9f2b305..ca770b2f1a3 100644
--- a/src/core/ext/filters/client_channel/client_channel.cc
+++ b/src/core/ext/filters/client_channel/client_channel.cc
@@ -3163,7 +3163,7 @@ void grpc_client_channel_populate_child_refs(
grpc_channel_element* elem, grpc_core::ChildRefsList* child_subchannels,
grpc_core::ChildRefsList* child_channels) {
channel_data* chand = static_cast<channel_data*>(elem->channel_data);
- if (chand->lb_policy) {
+ if (chand->lb_policy != nullptr) {
chand->lb_policy->FillChildRefsForChannelz(child_subchannels,
child_channels);
}
diff --git a/src/core/ext/filters/client_channel/client_channel.h b/src/core/ext/filters/client_channel/client_channel.h
index 0f95f6670a6..0b44a17562f 100644
--- a/src/core/ext/filters/client_channel/client_channel.h
+++ b/src/core/ext/filters/client_channel/client_channel.h
@@ -25,7 +25,6 @@
#include "src/core/ext/filters/client_channel/client_channel_factory.h"
#include "src/core/ext/filters/client_channel/resolver.h"
#include "src/core/lib/channel/channel_stack.h"
-#include "src/core/lib/json/json.h"
extern grpc_core::TraceFlag grpc_client_channel_trace;
diff --git a/src/core/ext/filters/client_channel/lb_policy.cc b/src/core/ext/filters/client_channel/lb_policy.cc
index a0d7758f235..d7b3ff6bb61 100644
--- a/src/core/ext/filters/client_channel/lb_policy.cc
+++ b/src/core/ext/filters/client_channel/lb_policy.cc
@@ -41,18 +41,6 @@ LoadBalancingPolicy::~LoadBalancingPolicy() {
GRPC_COMBINER_UNREF(combiner_, "lb_policy");
}
-void LoadBalancingPolicy::FillChildRefsForChannelz(
- ChildRefsList* child_subchannels, ChildRefsList* child_channels) {
- mu_guard guard(&child_refs_mu_);
- // TODO, de dup these.
- for (size_t i = 0; i < child_subchannels_.size(); ++i) {
- child_subchannels->push_back(child_subchannels_[i]);
- }
- for (size_t i = 0; i < child_channels_.size(); ++i) {
- child_channels->push_back(child_channels_[i]);
- }
-}
-
void LoadBalancingPolicy::TryReresolutionLocked(
grpc_core::TraceFlag* grpc_lb_trace, grpc_error* error) {
if (request_reresolution_ != nullptr) {
diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h
index 0c5fd2ad9cf..e756c89208f 100644
--- a/src/core/ext/filters/client_channel/lb_policy.h
+++ b/src/core/ext/filters/client_channel/lb_policy.h
@@ -29,7 +29,6 @@
#include "src/core/lib/gprpp/ref_counted_ptr.h"
#include "src/core/lib/iomgr/combiner.h"
#include "src/core/lib/iomgr/polling_entity.h"
-#include "src/core/lib/json/json.h"
#include "src/core/lib/transport/connectivity_state.h"
extern grpc_core::DebugOnlyTraceFlag grpc_trace_lb_policy_refcount;
@@ -145,6 +144,14 @@ class LoadBalancingPolicy
/// consider whether this method is still needed.
virtual void ExitIdleLocked() GRPC_ABSTRACT;
+ /// populates child_subchannels and child_channels with the uuids of this
+ /// LB policy's referenced children. This is not invoked from the
+ /// client_channel's combiner. The implementation is responsible for
+ /// providing its own synchronization.
+ virtual void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
+ ChildRefsList* child_channels)
+ GRPC_ABSTRACT;
+
void Orphan() override {
// Invoke ShutdownAndUnrefLocked() inside of the combiner.
GRPC_CLOSURE_SCHED(
@@ -159,13 +166,6 @@ class LoadBalancingPolicy
request_reresolution_ = request_reresolution;
}
- /// populates child_subchannels and child_channels with the uuids of this
- /// LB policies referenced children. This is not invoked from the
- /// client_channel's combiner. It has its own synchronization. This is
- /// not abstract, since the behavior is the same for all LB policies.
- void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
- ChildRefsList* child_channels);
-
grpc_pollset_set* interested_parties() const { return interested_parties_; }
GRPC_ABSTRACT_BASE_CLASS
diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index e43d1c2ab46..ab56922aa58 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -37,15 +37,6 @@ TraceFlag grpc_lb_pick_first_trace(false, "pick_first");
namespace {
-class LockGuard {
- public:
- LockGuard(gpr_mu* mu) : mu_(mu) { gpr_mu_lock(mu_); }
- ~LockGuard() { gpr_mu_unlock(mu_); }
-
- private:
- gpr_mu* mu_;
-};
-
//
// pick_first LB policy
//
@@ -112,10 +103,13 @@ class PickFirst : public LoadBalancingPolicy {
}
};
- class UpdateGuard {
+ // Helper class to ensure that any function that modifies the child refs
+ // data structures will update the channelz snapshot data structures before
+ // returning.
+ class AutoChildRefsUpdater {
public:
- UpdateGuard(PickFirst* pf) : pf_(pf) {}
- ~UpdateGuard() { pf_->UpdateChildRefsLocked(); }
+ explicit AutoChildRefsUpdater(PickFirst* pf) : pf_(pf) {}
+ ~AutoChildRefsUpdater() { pf_->UpdateChildRefsLocked(); }
private:
PickFirst* pf_;
@@ -177,7 +171,7 @@ void PickFirst::HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) {
}
void PickFirst::ShutdownLocked() {
- UpdateGuard(this);
+ AutoChildRefsUpdater(this);
grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Channel shutdown");
if (grpc_lb_pick_first_trace.enabled()) {
gpr_log(GPR_INFO, "Pick First %p Shutting down", this);
@@ -330,7 +324,7 @@ void PickFirst::UpdateChildRefsLocked() {
}
void PickFirst::UpdateLocked(const grpc_channel_args& args) {
- UpdateGuard guard(this);
+ AutoChildRefsUpdater guard(this);
const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES);
if (arg == nullptr || arg->type != GRPC_ARG_POINTER) {
if (subchannel_list_ == nullptr) {
@@ -438,7 +432,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) {
void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked(
grpc_connectivity_state connectivity_state, grpc_error* error) {
PickFirst* p = static_cast<PickFirst*>(subchannel_list()->policy());
- UpdateGuard guard(p);
+ AutoChildRefsUpdater guard(p);
// The notification must be for a subchannel in either the current or
// latest pending subchannel lists.
GPR_ASSERT(subchannel_list() == p->subchannel_list_.get() ||
diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc
index ffc7ff884cd..d9bce986d44 100644
--- a/src/core/lib/channel/channelz.cc
+++ b/src/core/lib/channel/channelz.cc
@@ -115,7 +115,6 @@ char* ChannelNode::RenderJSON() {
json_iterator =
grpc_json_create_child(json_iterator, json, "lastCallStartedTimestamp",
gpr_format_timespec(ts), GRPC_JSON_STRING, true);
-
json = top_level_json;
json_iterator = nullptr;
PopulateChildRefs(json);
diff --git a/src/core/lib/gpr/string.cc b/src/core/lib/gpr/string.cc
index 9bcb6714a39..0a76fc1f54a 100644
--- a/src/core/lib/gpr/string.cc
+++ b/src/core/lib/gpr/string.cc
@@ -56,12 +56,6 @@ typedef struct {
char* data;
} dump_out;
-// Returns an allocated string that represents tm according to RFC-3339, and,
-// more specifically, follows:
-// https://developers.google.com/protocol-buffers/docs/proto3#json
-//
-// "Uses RFC 3339, where generated output will always be Z-normalized and uses
-// 0, 3, 6 or 9 fractional digits."
char* gpr_format_timespec(gpr_timespec tm) {
char time_buffer[35];
char ns_buffer[11]; // '.' + 9 digits of precision
diff --git a/src/core/lib/gpr/string.h b/src/core/lib/gpr/string.h
index 2e8a4898d9d..ce51fe46321 100644
--- a/src/core/lib/gpr/string.h
+++ b/src/core/lib/gpr/string.h
@@ -21,6 +21,8 @@
#include <grpc/support/port_platform.h>
+#include <grpc/impl/codegen/gpr_types.h>
+
#include <stdbool.h>
#include <stddef.h>
@@ -81,6 +83,14 @@ char* gpr_strjoin_sep(const char** strs, size_t nstrs, const char* sep,
void gpr_string_split(const char* input, const char* sep, char*** strs,
size_t* nstrs);
+/* Returns an allocated string that represents tm according to RFC-3339, and,
+ more specifically, follows:
+ https://developers.google.com/protocol-buffers/docs/proto3#json
+
+ Uses RFC 3339, where generated output will always be Z-normalized and uses
+ 0, 3, 6 or 9 fractional digits. */
+char* gpr_format_timespec(gpr_timespec);
+
/* A vector of strings... for building up a final string one piece at a time */
typedef struct {
char** strs;
diff --git a/src/core/lib/gprpp/abstract.h b/src/core/lib/gprpp/abstract.h
index cc96edc49b7..5b7018e07e0 100644
--- a/src/core/lib/gprpp/abstract.h
+++ b/src/core/lib/gprpp/abstract.h
@@ -28,7 +28,10 @@
// gRPC currently can't depend on libstdc++, so we can't use "= 0" for
// pure virtual methods. Instead, we use this macro.
-#define GRPC_ABSTRACT \
- { GPR_ASSERT(false); }
+#define GRPC_ABSTRACT \
+ { \
+ gpr_log(GPR_ERROR, "Function marked GRPC_ABSTRACT was not implemented"); \
+ GPR_ASSERT(false); \
+ }
#endif /* GRPC_CORE_LIB_GPRPP_ABSTRACT_H */
diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc
index 8e7205f5586..90ed34da11f 100644
--- a/src/core/lib/iomgr/error.cc
+++ b/src/core/lib/iomgr/error.cc
@@ -670,8 +670,7 @@ static void collect_times_kvs(grpc_error* err, kv_pairs* kvs) {
uint8_t slot = err->times[which];
if (slot != UINT8_MAX) {
append_kv(kvs, key_time(static_cast<grpc_error_times>(which)),
- gpr_format_timespec(
- *reinterpret_cast<gpr_timespec*>(err->arena + slot)));
+ fmt_time(*reinterpret_cast<gpr_timespec*>(err->arena + slot)));
}
}
}
diff --git a/src/core/lib/json/json.h b/src/core/lib/json/json.h
index 8742774b5fc..8173845c723 100644
--- a/src/core/lib/json/json.h
+++ b/src/core/lib/json/json.h
@@ -91,7 +91,8 @@ grpc_json* grpc_json_create_child(grpc_json* sibling, grpc_json* parent,
const char* key, const char* value,
grpc_json_type type, bool owns_value);
-/* TODO */
+/* Creates a child json string object from the integer num, then links the
+ json object into the parent's json tree */
grpc_json* grpc_json_add_number_string_child(grpc_json* parent, grpc_json* it,
const char* name, int64_t num);
From 82e9cb66ffb3fa3b7dc151efa056ded997186a59 Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Thu, 12 Jul 2018 17:42:36 -0700
Subject: [PATCH 03/14] Reviewer feedback; move lists to children
---
.../ext/filters/client_channel/lb_policy.cc | 5 +--
.../ext/filters/client_channel/lb_policy.h | 8 -----
.../client_channel/lb_policy/grpclb/grpclb.cc | 2 ++
.../lb_policy/pick_first/pick_first.cc | 32 ++++++++++++++++---
.../lb_policy/round_robin/round_robin.cc | 2 ++
5 files changed, 32 insertions(+), 17 deletions(-)
diff --git a/src/core/ext/filters/client_channel/lb_policy.cc b/src/core/ext/filters/client_channel/lb_policy.cc
index d7b3ff6bb61..e065f45639b 100644
--- a/src/core/ext/filters/client_channel/lb_policy.cc
+++ b/src/core/ext/filters/client_channel/lb_policy.cc
@@ -31,13 +31,10 @@ LoadBalancingPolicy::LoadBalancingPolicy(const Args& args)
combiner_(GRPC_COMBINER_REF(args.combiner, "lb_policy")),
client_channel_factory_(args.client_channel_factory),
interested_parties_(grpc_pollset_set_create()),
- request_reresolution_(nullptr) {
- gpr_mu_init(&child_refs_mu_);
-}
+ request_reresolution_(nullptr) {}
LoadBalancingPolicy::~LoadBalancingPolicy() {
grpc_pollset_set_destroy(interested_parties_);
- gpr_mu_destroy(&child_refs_mu_);
GRPC_COMBINER_UNREF(combiner_, "lb_policy");
}
diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h
index e756c89208f..d525721ec03 100644
--- a/src/core/ext/filters/client_channel/lb_policy.h
+++ b/src/core/ext/filters/client_channel/lb_policy.h
@@ -180,9 +180,6 @@ class LoadBalancingPolicy
grpc_client_channel_factory* client_channel_factory() const {
return client_channel_factory_;
}
- gpr_mu* child_refs_mu() { return &child_refs_mu_; }
- ChildRefsList* child_subchannels() { return &child_subchannels_; }
- ChildRefsList* child_channels() { return &child_channels_; }
/// Shuts down the policy. Any pending picks that have not been
/// handed off to a new policy via HandOffPendingPicksLocked() will be
@@ -202,11 +199,6 @@ class LoadBalancingPolicy
/// Combiner under which LB policy actions take place.
grpc_combiner* combiner_;
- /// Lock and data used to capture snapshots of this channels child
- /// channels and subchannels. This data is consumed by channelz.
- gpr_mu child_refs_mu_;
- ChildRefsList child_subchannels_;
- ChildRefsList child_channels_;
/// Client channel factory, used to create channels and subchannels.
grpc_client_channel_factory* client_channel_factory_;
/// Owned pointer to interested parties in load balancing decisions.
diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
index 263b51ae895..622c03a8d15 100644
--- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
@@ -135,6 +135,8 @@ class GrpcLb : public LoadBalancingPolicy {
void HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) override;
void PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) override;
void ExitIdleLocked() override;
+ void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
+ ChildRefsList* child_channels) override {}
private:
/// Linked list of pending pick requests. It stores all information needed to
diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index ab56922aa58..7845c30f5b3 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -58,6 +58,8 @@ class PickFirst : public LoadBalancingPolicy {
void HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) override;
void PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) override;
void ExitIdleLocked() override;
+ void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
+ ChildRefsList* child_channels) override;
private:
~PickFirst();
@@ -135,10 +137,17 @@ class PickFirst : public LoadBalancingPolicy {
PickState* pending_picks_ = nullptr;
// Our connectivity state tracker.
grpc_connectivity_state_tracker state_tracker_;
+
+ /// Lock and data used to capture snapshots of this channels child
+ /// channels and subchannels. This data is consumed by channelz.
+ gpr_mu child_refs_mu_;
+ ChildRefsList child_subchannels_;
+ ChildRefsList child_channels_;
};
PickFirst::PickFirst(const Args& args) : LoadBalancingPolicy(args) {
GPR_ASSERT(args.client_channel_factory != nullptr);
+ gpr_mu_init(&child_refs_mu_);
grpc_connectivity_state_init(&state_tracker_, GRPC_CHANNEL_IDLE,
"pick_first");
if (grpc_lb_pick_first_trace.enabled()) {
@@ -152,6 +161,7 @@ PickFirst::~PickFirst() {
if (grpc_lb_pick_first_trace.enabled()) {
gpr_log(GPR_INFO, "Destroying Pick First %p", this);
}
+ gpr_mu_destroy(&child_refs_mu_);
GPR_ASSERT(subchannel_list_ == nullptr);
GPR_ASSERT(latest_pending_subchannel_list_ == nullptr);
GPR_ASSERT(pending_picks_ == nullptr);
@@ -294,19 +304,31 @@ void PickFirst::PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) {
}
}
+void PickFirst::FillChildRefsForChannelz(ChildRefsList* child_subchannels,
+ ChildRefsList* child_channels) {
+ mu_guard guard(&child_refs_mu_);
+ // TODO, de dup these.
+ for (size_t i = 0; i < child_subchannels_.size(); ++i) {
+ child_subchannels->push_back(child_subchannels_[i]);
+ }
+ for (size_t i = 0; i < child_channels_.size(); ++i) {
+ child_channels->push_back(child_channels_[i]);
+ }
+}
+
void PickFirst::UpdateChildRefsLocked() {
- mu_guard guard(child_refs_mu());
+ mu_guard guard(&child_refs_mu_);
// reset both lists
- child_subchannels()->clear();
+ child_subchannels_.clear();
// this will stay empty, because pick_first channels have no children
// channels.
- child_channels()->clear();
+ child_channels_.clear();
// populate the subchannels with boths subchannels lists, they will be
// deduped when the actual channelz query comes in.
if (subchannel_list_ != nullptr) {
for (size_t i = 0; i < subchannel_list_->num_subchannels(); ++i) {
if (subchannel_list_->subchannel(i)->subchannel() != nullptr) {
- child_subchannels()->push_back(grpc_subchannel_get_uuid(
+ child_subchannels_.push_back(grpc_subchannel_get_uuid(
subchannel_list_->subchannel(i)->subchannel()));
}
}
@@ -316,7 +338,7 @@ void PickFirst::UpdateChildRefsLocked() {
++i) {
if (latest_pending_subchannel_list_->subchannel(i)->subchannel() !=
nullptr) {
- child_subchannels()->push_back(grpc_subchannel_get_uuid(
+ child_subchannels_.push_back(grpc_subchannel_get_uuid(
latest_pending_subchannel_list_->subchannel(i)->subchannel()));
}
}
diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
index b1773850653..e6bc94a008a 100644
--- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
@@ -69,6 +69,8 @@ class RoundRobin : public LoadBalancingPolicy {
void HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) override;
void PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) override;
void ExitIdleLocked() override;
+ void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
+ ChildRefsList* child_channels) override {}
private:
~RoundRobin();
From 2d2854a1ce398c2a5c4693c68fe689055dda0cdd Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Fri, 13 Jul 2018 10:35:46 -0700
Subject: [PATCH 04/14] Add copy and move ctor to InlinedVector
---
src/core/lib/gprpp/inlined_vector.h | 73 +++++++++++++++++-
test/core/gprpp/inlined_vector_test.cc | 101 ++++++++++++++++++++-----
2 files changed, 153 insertions(+), 21 deletions(-)
diff --git a/src/core/lib/gprpp/inlined_vector.h b/src/core/lib/gprpp/inlined_vector.h
index 0d2586e507d..276d5a078d4 100644
--- a/src/core/lib/gprpp/inlined_vector.h
+++ b/src/core/lib/gprpp/inlined_vector.h
@@ -50,9 +50,76 @@ class InlinedVector {
InlinedVector() { init_data(); }
~InlinedVector() { destroy_elements(); }
- // For now, we do not support copying.
- InlinedVector(const InlinedVector&) = delete;
- InlinedVector& operator=(const InlinedVector&) = delete;
+ // copy constructors
+ InlinedVector(const InlinedVector& v) {
+ init_data();
+ // if v is allocated, then we copy it's buffer
+ if (v.dynamic_ != nullptr) {
+ reserve(v.capacity_);
+ memcpy(dynamic_, v.dynamic_, v.capacity_ * sizeof(T));
+ } else {
+ memcpy(inline_, v.inline_, v.capacity_ * sizeof(T));
+ dynamic_ = nullptr;
+ }
+ // copy over metadata
+ size_ = v.size_;
+ capacity_ = v.capacity_;
+ }
+ InlinedVector& operator=(const InlinedVector& v) {
+ if (this != &v) {
+ clear();
+ // if v is allocated, then we copy it's buffer
+ if (v.dynamic_ != nullptr) {
+ reserve(v.capacity_);
+ memcpy(dynamic_, v.dynamic_, v.capacity_ * sizeof(T));
+ } else {
+ memcpy(inline_, v.inline_, v.capacity_ * sizeof(T));
+ dynamic_ = nullptr;
+ }
+ // copy over metadata
+ size_ = v.size_;
+ capacity_ = v.capacity_;
+ }
+ return *this;
+ }
+
+ // move constructors
+ InlinedVector(InlinedVector&& v) {
+ // if v is allocated, then we steal it's buffer
+ if (v.dynamic_ != nullptr) {
+ dynamic_ = v.dynamic_;
+ } else {
+ memcpy(inline_, v.inline_, v.capacity_ * sizeof(T));
+ dynamic_ = nullptr;
+ }
+ // copy over metadata
+ size_ = v.size_;
+ capacity_ = v.capacity_;
+ // null out the original
+ v.dynamic_ = nullptr;
+ v.size_ = 0;
+ v.capacity_ = 0;
+ }
+ InlinedVector& operator=(InlinedVector&& v) {
+ if (this != &v) {
+ clear();
+ // if v is allocated, then we steal it's buffer
+ if (v.dynamic_ != nullptr) {
+ dynamic_ = v.dynamic_;
+ } else {
+ memcpy(inline_, v.inline_, v.capacity_ * sizeof(T));
+ dynamic_ = nullptr;
+ }
+ // copy over metadata
+ size_ = v.size_;
+ capacity_ = v.capacity_;
+ // null out the original
+ v.dynamic_ = nullptr;
+ v.size_ = 0;
+ v.capacity_ = 0;
+ }
+ return *this;
+ }
T* data() {
return dynamic_ != nullptr ? dynamic_ : reinterpret_cast<T*>(inline_);
diff --git a/test/core/gprpp/inlined_vector_test.cc b/test/core/gprpp/inlined_vector_test.cc
index 41f4338f8a4..ba2f4d9e6f2 100644
--- a/test/core/gprpp/inlined_vector_test.cc
+++ b/test/core/gprpp/inlined_vector_test.cc
@@ -17,20 +17,30 @@
*/
#include "src/core/lib/gprpp/inlined_vector.h"
+#include <grpc/support/log.h>
#include <gtest/gtest.h>
#include "src/core/lib/gprpp/memory.h"
#include "test/core/util/test_config.h"
namespace grpc_core {
namespace testing {
+namespace {
+
+template <typename Vector>
+static void FillVector(Vector* v, int len, int offset = 0) {
+ for (int i = 0; i < len; i++) {
+ v->push_back(i + offset);
+ EXPECT_EQ(i + 1UL, v->size());
+ }
+}
+
+} // namespace
TEST(InlinedVectorTest, CreateAndIterate) {
const int kNumElements = 9;
InlinedVector<int, 2> v;
EXPECT_TRUE(v.empty());
- for (int i = 0; i < kNumElements; ++i) {
- v.push_back(i);
- }
+ FillVector(&v, kNumElements);
EXPECT_EQ(static_cast<size_t>(kNumElements), v.size());
EXPECT_FALSE(v.empty());
for (int i = 0; i < kNumElements; ++i) {
@@ -42,9 +52,7 @@ TEST(InlinedVectorTest, CreateAndIterate) {
TEST(InlinedVectorTest, ValuesAreInlined) {
const int kNumElements = 5;
InlinedVector<int, 10> v;
- for (int i = 0; i < kNumElements; ++i) {
- v.push_back(i);
- }
+ FillVector(&v, kNumElements);
EXPECT_EQ(static_cast<size_t>(kNumElements), v.size());
for (int i = 0; i < kNumElements; ++i) {
EXPECT_EQ(i, v[i]);
@@ -71,19 +79,13 @@ TEST(InlinedVectorTest, ClearAndRepopulate) {
const int kNumElements = 10;
InlinedVector<int, 5> v;
EXPECT_EQ(0UL, v.size());
- for (int i = 0; i < kNumElements; ++i) {
- v.push_back(i);
- EXPECT_EQ(i + 1UL, v.size());
- }
+ FillVector(&v, kNumElements);
for (int i = 0; i < kNumElements; ++i) {
EXPECT_EQ(i, v[i]);
}
v.clear();
EXPECT_EQ(0UL, v.size());
- for (int i = 0; i < kNumElements; ++i) {
- v.push_back(kNumElements + i);
- EXPECT_EQ(i + 1UL, v.size());
- }
+ FillVector(&v, kNumElements, kNumElements);
for (int i = 0; i < kNumElements; ++i) {
EXPECT_EQ(kNumElements + i, v[i]);
}
@@ -93,10 +95,7 @@ TEST(InlinedVectorTest, ConstIndexOperator) {
constexpr int kNumElements = 10;
InlinedVector<int, 5> v;
EXPECT_EQ(0UL, v.size());
- for (int i = 0; i < kNumElements; ++i) {
- v.push_back(i);
- EXPECT_EQ(i + 1UL, v.size());
- }
+ FillVector(&v, kNumElements);
// The following lambda function is exceptionally allowed to use an anonymous
// capture due to the erroneous behavior of the MSVC compiler, that refuses to
// capture the kNumElements constexpr, something allowed by the standard.
@@ -108,6 +107,72 @@ TEST(InlinedVectorTest, ConstIndexOperator) {
const_func(v);
}
+TEST(InlinedVectorTest, CopyConstructorAndAssignment) {
+ typedef InlinedVector<int, 8> IntVec8;
+ for (size_t len = 0; len < 20; len++) {
+ IntVec8 original;
+ FillVector(&original, len);
+ EXPECT_EQ(len, original.size());
+ EXPECT_LE(len, original.capacity());
+
+ IntVec8 copy_constructed(original);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_TRUE(original[i] == copy_constructed[i]);
+ }
+
+ for (size_t start_len = 0; start_len < 20; start_len++) {
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, start_len, 99); // Add dummy elements
+ copy_assigned = original;
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_TRUE(original[i] == copy_assigned[i]);
+ }
+ }
+ }
+}
+
+TEST(InlinedVectorTest, MoveConstructorAndAssignment) {
+ typedef InlinedVector<int, 8> IntVec8;
+ for (size_t len = 0; len < 20; len++) {
+ IntVec8 original;
+ const size_t inlined_capacity = original.capacity();
+ FillVector(&original, len);
+ EXPECT_EQ(len, original.size());
+ EXPECT_LE(len, original.capacity());
+
+ {
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ IntVec8 move_constructed(std::move(tmp));
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_TRUE(original[i] == move_constructed[i]);
+ }
+ if (original.size() > inlined_capacity) {
+ // Allocation is moved as a whole, data stays in place.
+ EXPECT_TRUE(move_constructed.data() == old_data);
+ } else {
+ EXPECT_FALSE(move_constructed.data() == old_data);
+ }
+ }
+ for (size_t start_len = 0; start_len < 20; start_len++) {
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, start_len, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_TRUE(original[i] == move_assigned[i]);
+ }
+ if (original.size() > inlined_capacity) {
+ // Allocation is moved as a whole, data stays in place.
+ EXPECT_TRUE(move_assigned.data() == old_data);
+ } else {
+ EXPECT_FALSE(move_assigned.data() == old_data);
+ }
+ }
+ }
+}
+
} // namespace testing
} // namespace grpc_core
From 9ff83ea77ab5fecfd7b57781f16b5d951b70b7c1 Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Fri, 13 Jul 2018 10:51:37 -0700
Subject: [PATCH 05/14] Reviewer feedback; less locking, slow de dup
---
.../lb_policy/pick_first/pick_first.cc | 38 ++++++++++---------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index 7845c30f5b3..1ecae1690c7 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -304,31 +304,31 @@ void PickFirst::PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) {
}
}
-void PickFirst::FillChildRefsForChannelz(ChildRefsList* child_subchannels,
- ChildRefsList* child_channels) {
+void PickFirst::FillChildRefsForChannelz(
+ ChildRefsList* child_subchannels_to_fill, ChildRefsList* ignored) {
mu_guard guard(&child_refs_mu_);
- // TODO, de dup these.
for (size_t i = 0; i < child_subchannels_.size(); ++i) {
- child_subchannels->push_back(child_subchannels_[i]);
- }
- for (size_t i = 0; i < child_channels_.size(); ++i) {
- child_channels->push_back(child_channels_[i]);
+ // TODO(ncteisen): implement a de dup loop that is not O(n^2). Might
+ // have to implement lightweight set. For now, we don't care about
+ // performance when channelz requests are made.
+ bool add_elt = true;
+ for (size_t j = 0; j < child_subchannels_to_fill->size(); ++j) {
+ if ((*child_subchannels_to_fill)[j] == child_subchannels_[i]) {
+ add_elt = false;
+ }
+ }
+ if (add_elt) {
+ child_subchannels_to_fill->push_back(child_subchannels_[i]);
+ }
}
}
void PickFirst::UpdateChildRefsLocked() {
- mu_guard guard(&child_refs_mu_);
- // reset both lists
- child_subchannels_.clear();
- // this will stay empty, because pick_first channels have no children
- // channels.
- child_channels_.clear();
- // populate the subchannels with boths subchannels lists, they will be
- // deduped when the actual channelz query comes in.
+ ChildRefsList cs;
if (subchannel_list_ != nullptr) {
for (size_t i = 0; i < subchannel_list_->num_subchannels(); ++i) {
if (subchannel_list_->subchannel(i)->subchannel() != nullptr) {
- child_subchannels_.push_back(grpc_subchannel_get_uuid(
+ cs.push_back(grpc_subchannel_get_uuid(
subchannel_list_->subchannel(i)->subchannel()));
}
}
@@ -338,11 +338,15 @@ void PickFirst::UpdateChildRefsLocked() {
++i) {
if (latest_pending_subchannel_list_->subchannel(i)->subchannel() !=
nullptr) {
- child_subchannels_.push_back(grpc_subchannel_get_uuid(
+ cs.push_back(grpc_subchannel_get_uuid(
latest_pending_subchannel_list_->subchannel(i)->subchannel()));
}
}
}
+
+ // atomically update the data that channelz will actually be looking at.
+ mu_guard guard(&child_refs_mu_);
+ child_subchannels_ = std::move(cs);
}
void PickFirst::UpdateLocked(const grpc_channel_args& args) {
From 1e6c0b46bca8a57d0c3725107d1b6508ebf1e1c3 Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Fri, 13 Jul 2018 11:22:08 -0700
Subject: [PATCH 06/14] Ensure subchannel channelz only created if enabled
---
.../lb_policy/pick_first/pick_first.cc | 16 ++++++++++----
.../ext/filters/client_channel/subchannel.cc | 21 ++++++++++++-------
.../ext/filters/client_channel/subchannel.h | 7 ++++++-
src/core/lib/channel/channelz.cc | 8 +++++++
src/core/lib/channel/channelz.h | 18 ++++++++++++++++
5 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index 1ecae1690c7..390b4ee1bbe 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -328,8 +328,12 @@ void PickFirst::UpdateChildRefsLocked() {
if (subchannel_list_ != nullptr) {
for (size_t i = 0; i < subchannel_list_->num_subchannels(); ++i) {
if (subchannel_list_->subchannel(i)->subchannel() != nullptr) {
- cs.push_back(grpc_subchannel_get_uuid(
- subchannel_list_->subchannel(i)->subchannel()));
+ grpc_core::channelz::SubchannelNode* subchannel_node =
+ grpc_subchannel_get_channelz_node(
+ subchannel_list_->subchannel(i)->subchannel());
+ if (subchannel_node != nullptr) {
+ cs.push_back(subchannel_node->subchannel_uuid());
+ }
}
}
}
@@ -338,8 +342,12 @@ void PickFirst::UpdateChildRefsLocked() {
++i) {
if (latest_pending_subchannel_list_->subchannel(i)->subchannel() !=
nullptr) {
- cs.push_back(grpc_subchannel_get_uuid(
- latest_pending_subchannel_list_->subchannel(i)->subchannel()));
+ grpc_core::channelz::SubchannelNode* subchannel_node =
+ grpc_subchannel_get_channelz_node(
+ latest_pending_subchannel_list_->subchannel(i)->subchannel());
+ if (subchannel_node != nullptr) {
+ cs.push_back(subchannel_node->subchannel_uuid());
+ }
}
}
}
diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc
index 6c4f1869eae..0e349da3e21 100644
--- a/src/core/ext/filters/client_channel/subchannel.cc
+++ b/src/core/ext/filters/client_channel/subchannel.cc
@@ -135,10 +135,8 @@ struct grpc_subchannel {
/** our alarm */
grpc_timer alarm;
- /* the global uuid for this subchannel */
- // TODO(ncteisen): move this into SubchannelNode while implementing
- // GetSubchannel.
- intptr_t subchannel_uuid;
+ grpc_core::RefCountedPtr<grpc_core::channelz::SubchannelNode>
+ channelz_subchannel;
};
struct grpc_subchannel_call {
@@ -379,14 +377,21 @@ grpc_subchannel* grpc_subchannel_create(grpc_connector* connector,
c->backoff.Init(backoff_options);
gpr_mu_init(&c->mu);
- // This is just a placeholder for now
- c->subchannel_uuid = 42;
+ // This is just a placeholder channelz class for for now.
+ const grpc_arg* arg =
+ grpc_channel_args_find(c->args, GRPC_ARG_ENABLE_CHANNELZ);
+ bool channelz_enabled = grpc_channel_arg_get_bool(arg, false);
+ if (channelz_enabled) {
+ c->channelz_subchannel =
+ grpc_core::MakeRefCounted<grpc_core::channelz::SubchannelNode>();
+ }
return grpc_subchannel_index_register(key, c);
}
-intptr_t grpc_subchannel_get_uuid(grpc_subchannel* s) {
- return s->subchannel_uuid;
+grpc_core::channelz::SubchannelNode* grpc_subchannel_get_channelz_node(
+ grpc_subchannel* s) {
+ return s->channelz_subchannel.get();
}
static void continue_connect_locked(grpc_subchannel* c) {
diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h
index 590e80f507e..f76be815437 100644
--- a/src/core/ext/filters/client_channel/subchannel.h
+++ b/src/core/ext/filters/client_channel/subchannel.h
@@ -71,6 +71,10 @@ typedef struct grpc_subchannel_key grpc_subchannel_key;
namespace grpc_core {
+namespace channelz {
+class SubchannelNode;
+}
+
class ConnectedSubchannel : public RefCountedWithTracing<ConnectedSubchannel> {
public:
struct CallArgs {
@@ -115,7 +119,8 @@ grpc_subchannel_call* grpc_subchannel_call_ref(
void grpc_subchannel_call_unref(
grpc_subchannel_call* call GRPC_SUBCHANNEL_REF_EXTRA_ARGS);
-intptr_t grpc_subchannel_get_uuid(grpc_subchannel* subchannel);
+grpc_core::channelz::SubchannelNode* grpc_subchannel_get_channelz_node(
+ 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/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc
index d9bce986d44..5a0620c51f3 100644
--- a/src/core/lib/channel/channelz.cc
+++ b/src/core/lib/channel/channelz.cc
@@ -131,5 +131,13 @@ RefCountedPtr<ChannelNode> ChannelNode::MakeChannelNode(
channel, channel_tracer_max_nodes);
}
+SubchannelNode::SubchannelNode() : subchannel_uuid_(-1) {
+ subchannel_uuid_ = ChannelzRegistry::Register(this);
+}
+
+SubchannelNode::~SubchannelNode() {
+ ChannelzRegistry::Unregister(subchannel_uuid_);
+}
+
} // namespace channelz
} // namespace grpc_core
diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h
index e84c187f11a..f2c5ecddbfe 100644
--- a/src/core/lib/channel/channelz.h
+++ b/src/core/lib/channel/channelz.h
@@ -95,6 +95,24 @@ class ChannelNode : public RefCounted<ChannelNode> {
ManualConstructor<ChannelTrace> trace_;
};
+// Placeholds channelz class for subchannels. All this can do now is track its
+// uuid (this information is needed by the parent channelz class). In the next
+// PR I will build this out to support the GetSubchannel channelz request.
+class SubchannelNode : public RefCounted<SubchannelNode> {
+ public:
+ SubchannelNode();
+ virtual ~SubchannelNode();
+
+ intptr_t subchannel_uuid() { return subchannel_uuid_; }
+
+ protected:
+ GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
+ GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
+
+ private:
+ intptr_t subchannel_uuid_;
+};
+
// Creation functions
typedef RefCountedPtr<ChannelNode> (*ChannelNodeCreationFunc)(grpc_channel*,
From 5fd07bd9ef72c2dcef3f0da159b41ca6a0a0f6b0 Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Fri, 13 Jul 2018 15:05:47 -0700
Subject: [PATCH 07/14] Fix sanity and build
---
grpc.def | 1 -
src/core/lib/gprpp/inlined_vector.h | 1 +
src/ruby/ext/grpc/rb_grpc_imports.generated.c | 2 --
src/ruby/ext/grpc/rb_grpc_imports.generated.h | 3 ---
test/core/surface/public_headers_must_be_c89.c | 1 -
5 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/grpc.def b/grpc.def
index 7d33e085754..06db74cad58 100644
--- a/grpc.def
+++ b/grpc.def
@@ -199,7 +199,6 @@ EXPORTS
gpr_format_message
gpr_strdup
gpr_asprintf
- gpr_format_timespec
gpr_mu_init
gpr_mu_destroy
gpr_mu_lock
diff --git a/src/core/lib/gprpp/inlined_vector.h b/src/core/lib/gprpp/inlined_vector.h
index 276d5a078d4..9c2e1314555 100644
--- a/src/core/lib/gprpp/inlined_vector.h
+++ b/src/core/lib/gprpp/inlined_vector.h
@@ -22,6 +22,7 @@
#include <grpc/support/port_platform.h>
#include <cassert>
+#include <cstring>
#include "src/core/lib/gprpp/memory.h"
diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c
index 1086b7dfcb9..4e235121e24 100644
--- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c
+++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c
@@ -222,7 +222,6 @@ gpr_cpu_current_cpu_type gpr_cpu_current_cpu_import;
gpr_format_message_type gpr_format_message_import;
gpr_strdup_type gpr_strdup_import;
gpr_asprintf_type gpr_asprintf_import;
-gpr_format_timespec_type gpr_format_timespec_import;
gpr_mu_init_type gpr_mu_init_import;
gpr_mu_destroy_type gpr_mu_destroy_import;
gpr_mu_lock_type gpr_mu_lock_import;
@@ -471,7 +470,6 @@ void grpc_rb_load_imports(HMODULE library) {
gpr_format_message_import = (gpr_format_message_type) GetProcAddress(library, "gpr_format_message");
gpr_strdup_import = (gpr_strdup_type) GetProcAddress(library, "gpr_strdup");
gpr_asprintf_import = (gpr_asprintf_type) GetProcAddress(library, "gpr_asprintf");
- gpr_format_timespec_import = (gpr_format_timespec_type) GetProcAddress(library, "gpr_format_timespec");
gpr_mu_init_import = (gpr_mu_init_type) GetProcAddress(library, "gpr_mu_init");
gpr_mu_destroy_import = (gpr_mu_destroy_type) GetProcAddress(library, "gpr_mu_destroy");
gpr_mu_lock_import = (gpr_mu_lock_type) GetProcAddress(library, "gpr_mu_lock");
diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h
index b4107641a48..f01c9c82482 100644
--- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h
+++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h
@@ -641,9 +641,6 @@ extern gpr_strdup_type gpr_strdup_import;
typedef int(*gpr_asprintf_type)(char** strp, const char* format, ...) GPR_PRINT_FORMAT_CHECK(2, 3);
extern gpr_asprintf_type gpr_asprintf_import;
#define gpr_asprintf gpr_asprintf_import
-typedef char*(*gpr_format_timespec_type)(gpr_timespec);
-extern gpr_format_timespec_type gpr_format_timespec_import;
-#define gpr_format_timespec gpr_format_timespec_import
typedef void(*gpr_mu_init_type)(gpr_mu* mu);
extern gpr_mu_init_type gpr_mu_init_import;
#define gpr_mu_init gpr_mu_init_import
diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c
index 95deb1f17a3..9a79b468dd5 100644
--- a/test/core/surface/public_headers_must_be_c89.c
+++ b/test/core/surface/public_headers_must_be_c89.c
@@ -257,7 +257,6 @@ int main(int argc, char **argv) {
printf("%lx", (unsigned long) gpr_cpu_current_cpu);
printf("%lx", (unsigned long) gpr_strdup);
printf("%lx", (unsigned long) gpr_asprintf);
- printf("%lx", (unsigned long) gpr_format_timespec);
printf("%lx", (unsigned long) gpr_mu_init);
printf("%lx", (unsigned long) gpr_mu_destroy);
printf("%lx", (unsigned long) gpr_mu_lock);
From 1f325781bced4f6311462cf4e7682aa99ea34d0b Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Sun, 15 Jul 2018 22:20:32 -0700
Subject: [PATCH 08/14] REVERT THIS: fix alignment to fix tests for now
---
src/core/lib/gprpp/memory.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/core/lib/gprpp/memory.h b/src/core/lib/gprpp/memory.h
index e90bedcd9b4..107f358091b 100644
--- a/src/core/lib/gprpp/memory.h
+++ b/src/core/lib/gprpp/memory.h
@@ -41,7 +41,7 @@
namespace grpc_core {
// The alignment of memory returned by gpr_malloc().
-constexpr size_t kAlignmentForDefaultAllocationInBytes = 8;
+constexpr size_t kAlignmentForDefaultAllocationInBytes = 16;
// Alternative to new, since we cannot use it (for fear of libstdc++)
template <typename T, typename... Args>
From d6c5c3c3e57fe4e2b687a23f29fd2ffab98ec10a Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Mon, 16 Jul 2018 09:16:54 -0700
Subject: [PATCH 09/14] Fix ASAN
---
src/core/ext/filters/client_channel/subchannel.cc | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc
index 0e349da3e21..86026b70a5e 100644
--- a/src/core/ext/filters/client_channel/subchannel.cc
+++ b/src/core/ext/filters/client_channel/subchannel.cc
@@ -181,6 +181,9 @@ static void connection_destroy(void* arg, grpc_error* error) {
static void subchannel_destroy(void* arg, grpc_error* error) {
grpc_subchannel* c = static_cast<grpc_subchannel*>(arg);
+ if (c->channelz_subchannel != nullptr) {
+ c->channelz_subchannel.reset();
+ }
gpr_free((void*)c->filters);
grpc_channel_args_destroy(c->args);
grpc_connectivity_state_destroy(&c->state_tracker);
From c0f110b398751375d9bf1aeaf3d8ae1812ba60e3 Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Mon, 16 Jul 2018 21:40:16 -0700
Subject: [PATCH 10/14] Revert "REVERT THIS: fix alignment to fix tests for
now"
This reverts commit 1f325781bced4f6311462cf4e7682aa99ea34d0b.
---
src/core/lib/gprpp/memory.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/core/lib/gprpp/memory.h b/src/core/lib/gprpp/memory.h
index 107f358091b..e90bedcd9b4 100644
--- a/src/core/lib/gprpp/memory.h
+++ b/src/core/lib/gprpp/memory.h
@@ -41,7 +41,7 @@
namespace grpc_core {
// The alignment of memory returned by gpr_malloc().
-constexpr size_t kAlignmentForDefaultAllocationInBytes = 16;
+constexpr size_t kAlignmentForDefaultAllocationInBytes = 8;
// Alternative to new, since we cannot use it (for fear of libstdc++)
template <typename T, typename... Args>
From d5bd4274660b603b802c542d6af264f72fbd034d Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Mon, 16 Jul 2018 21:43:59 -0700
Subject: [PATCH 11/14] Fix the alignment problem, still messy
---
src/core/ext/filters/client_channel/lb_policy.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h
index d525721ec03..7bc224ad811 100644
--- a/src/core/ext/filters/client_channel/lb_policy.h
+++ b/src/core/ext/filters/client_channel/lb_policy.h
@@ -205,6 +205,11 @@ class LoadBalancingPolicy
grpc_pollset_set* interested_parties_;
/// Callback to force a re-resolution.
grpc_closure* request_reresolution_;
+
+ // Dummy classes needed for alignment issues.
+ // See https://github.com/grpc/grpc/issues/16032 for context.
+ ChildRefsList dummy_list_foo;
+ ChildRefsList dummy_list_bar;
};
} // namespace grpc_core
From adfa81987af4b61eb11c92c6e4bedc3bed3028c9 Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Tue, 17 Jul 2018 09:35:56 -0700
Subject: [PATCH 12/14] Un-pare simple_request
---
test/core/end2end/tests/simple_request.cc | 32 +++++++++++------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc
index 7ec11d8ac3d..941d9ae3198 100644
--- a/test/core/end2end/tests/simple_request.cc
+++ b/test/core/end2end/tests/simple_request.cc
@@ -256,24 +256,24 @@ static void test_invoke_simple_request(grpc_end2end_test_config config) {
config.tear_down_data(&f);
}
-// static void test_invoke_10_simple_requests(grpc_end2end_test_config config) {
-// int i;
-// grpc_end2end_test_fixture f =
-// begin_test(config, "test_invoke_10_simple_requests", nullptr, nullptr);
-// for (i = 0; i < 10; i++) {
-// simple_request_body(config, f);
-// gpr_log(GPR_INFO, "Running test: Passed simple request %d", i);
-// }
-// end_test(&f);
-// config.tear_down_data(&f);
-// }
+static void test_invoke_10_simple_requests(grpc_end2end_test_config config) {
+ int i;
+ grpc_end2end_test_fixture f =
+ begin_test(config, "test_invoke_10_simple_requests", nullptr, nullptr);
+ for (i = 0; i < 10; i++) {
+ simple_request_body(config, f);
+ gpr_log(GPR_INFO, "Running test: Passed simple request %d", i);
+ }
+ end_test(&f);
+ config.tear_down_data(&f);
+}
void simple_request(grpc_end2end_test_config config) {
- // int i;
- // for (i = 0; i < 10; i++) {
- test_invoke_simple_request(config);
- // }
- // test_invoke_10_simple_requests(config);
+ int i;
+ for (i = 0; i < 10; i++) {
+ test_invoke_simple_request(config);
+ }
+ test_invoke_10_simple_requests(config);
}
void simple_request_pre_init(void) {}
From 0f6e4dd20d3e79d3465a7bf8a21d704e7b67b102 Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Tue, 17 Jul 2018 11:26:55 -0700
Subject: [PATCH 13/14] reviewer feedback:
---
.../client_channel/client_channel_channelz.h | 3 +
.../ext/filters/client_channel/lb_policy.h | 1 +
.../client_channel/lb_policy/grpclb/grpclb.cc | 1 +
.../lb_policy/pick_first/pick_first.cc | 8 +-
.../lb_policy/round_robin/round_robin.cc | 1 +
.../ext/filters/client_channel/subchannel.cc | 5 +-
.../ext/filters/client_channel/subchannel.h | 5 +-
src/core/lib/channel/channelz.cc | 2 +-
src/core/lib/channel/channelz.h | 4 +-
src/core/lib/gprpp/inlined_vector.h | 88 +++----
test/core/gprpp/inlined_vector_test.cc | 236 ++++++++++++++----
11 files changed, 232 insertions(+), 122 deletions(-)
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 e5be52e7785..0547109d36f 100644
--- a/src/core/ext/filters/client_channel/client_channel_channelz.h
+++ b/src/core/ext/filters/client_channel/client_channel_channelz.h
@@ -28,6 +28,9 @@
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<intptr_t, 10> ChildRefsList;
namespace channelz {
diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h
index 7bc224ad811..3150df8847b 100644
--- a/src/core/ext/filters/client_channel/lb_policy.h
+++ b/src/core/ext/filters/client_channel/lb_policy.h
@@ -208,6 +208,7 @@ class LoadBalancingPolicy
// Dummy classes needed for alignment issues.
// See https://github.com/grpc/grpc/issues/16032 for context.
+ // TODO(ncteisen): remove this as soon as the issue is resolved.
ChildRefsList dummy_list_foo;
ChildRefsList dummy_list_bar;
};
diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
index 622c03a8d15..f757d6057cd 100644
--- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc
@@ -135,6 +135,7 @@ class GrpcLb : public LoadBalancingPolicy {
void HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) override;
void PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) override;
void ExitIdleLocked() override;
+ // TODO(ncteisen): implement this in a follow up PR
void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
ChildRefsList* child_channels) override {}
diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index 390b4ee1bbe..c50deb96797 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -311,13 +311,14 @@ void PickFirst::FillChildRefsForChannelz(
// TODO(ncteisen): implement a de dup loop that is not O(n^2). Might
// have to implement lightweight set. For now, we don't care about
// performance when channelz requests are made.
- bool add_elt = true;
+ bool found = false;
for (size_t j = 0; j < child_subchannels_to_fill->size(); ++j) {
if ((*child_subchannels_to_fill)[j] == child_subchannels_[i]) {
- add_elt = false;
+ found = true;
+ break;
}
}
- if (add_elt) {
+ if (!found) {
child_subchannels_to_fill->push_back(child_subchannels_[i]);
}
}
@@ -351,7 +352,6 @@ void PickFirst::UpdateChildRefsLocked() {
}
}
}
-
// atomically update the data that channelz will actually be looking at.
mu_guard guard(&child_refs_mu_);
child_subchannels_ = std::move(cs);
diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
index e6bc94a008a..4c42b2063fc 100644
--- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
@@ -69,6 +69,7 @@ class RoundRobin : public LoadBalancingPolicy {
void HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) override;
void PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) override;
void ExitIdleLocked() override;
+ // TODO(ncteisen): implement this in a follow up PR
void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
ChildRefsList* child_channels) override {}
diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc
index 86026b70a5e..9d608c3c55d 100644
--- a/src/core/ext/filters/client_channel/subchannel.cc
+++ b/src/core/ext/filters/client_channel/subchannel.cc
@@ -181,9 +181,7 @@ static void connection_destroy(void* arg, grpc_error* error) {
static void subchannel_destroy(void* arg, grpc_error* error) {
grpc_subchannel* c = static_cast<grpc_subchannel*>(arg);
- if (c->channelz_subchannel != nullptr) {
- c->channelz_subchannel.reset();
- }
+ c->channelz_subchannel.reset();
gpr_free((void*)c->filters);
grpc_channel_args_destroy(c->args);
grpc_connectivity_state_destroy(&c->state_tracker);
@@ -380,7 +378,6 @@ grpc_subchannel* grpc_subchannel_create(grpc_connector* connector,
c->backoff.Init(backoff_options);
gpr_mu_init(&c->mu);
- // This is just a placeholder channelz class for for now.
const grpc_arg* arg =
grpc_channel_args_find(c->args, GRPC_ARG_ENABLE_CHANNELZ);
bool channelz_enabled = grpc_channel_arg_get_bool(arg, false);
diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h
index f76be815437..9e53f7d5423 100644
--- a/src/core/ext/filters/client_channel/subchannel.h
+++ b/src/core/ext/filters/client_channel/subchannel.h
@@ -21,6 +21,7 @@
#include <grpc/support/port_platform.h>
+#include "src/core/ext/filters/client_channel/client_channel_channelz.h"
#include "src/core/ext/filters/client_channel/connector.h"
#include "src/core/lib/channel/channel_stack.h"
#include "src/core/lib/gpr/arena.h"
@@ -71,10 +72,6 @@ typedef struct grpc_subchannel_key grpc_subchannel_key;
namespace grpc_core {
-namespace channelz {
-class SubchannelNode;
-}
-
class ConnectedSubchannel : public RefCountedWithTracing<ConnectedSubchannel> {
public:
struct CallArgs {
diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc
index 5a0620c51f3..79a9220503c 100644
--- a/src/core/lib/channel/channelz.cc
+++ b/src/core/lib/channel/channelz.cc
@@ -131,7 +131,7 @@ RefCountedPtr<ChannelNode> ChannelNode::MakeChannelNode(
channel, channel_tracer_max_nodes);
}
-SubchannelNode::SubchannelNode() : subchannel_uuid_(-1) {
+SubchannelNode::SubchannelNode() {
subchannel_uuid_ = ChannelzRegistry::Register(this);
}
diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h
index f2c5ecddbfe..7184af93ecb 100644
--- a/src/core/lib/channel/channelz.h
+++ b/src/core/lib/channel/channelz.h
@@ -96,8 +96,8 @@ class ChannelNode : public RefCounted<ChannelNode> {
};
// Placeholds channelz class for subchannels. All this can do now is track its
-// uuid (this information is needed by the parent channelz class). In the next
-// PR I will build this out to support the GetSubchannel channelz request.
+// uuid (this information is needed by the parent channelz class).
+// TODO(ncteisen): build this out to support the GetSubchannel channelz request.
class SubchannelNode : public RefCounted<SubchannelNode> {
public:
SubchannelNode();
diff --git a/src/core/lib/gprpp/inlined_vector.h b/src/core/lib/gprpp/inlined_vector.h
index 9c2e1314555..508fb2eed11 100644
--- a/src/core/lib/gprpp/inlined_vector.h
+++ b/src/core/lib/gprpp/inlined_vector.h
@@ -51,73 +51,30 @@ class InlinedVector {
InlinedVector() { init_data(); }
~InlinedVector() { destroy_elements(); }
- // copy constructors
+ // copy constructor
InlinedVector(const InlinedVector& v) {
init_data();
- // if v is allocated, then we copy it's buffer
- if (v.dynamic_ != nullptr) {
- reserve(v.capacity_);
- memcpy(dynamic_, v.dynamic_, v.capacity_ * sizeof(T));
- } else {
- memcpy(inline_, v.inline_, v.capacity_ * sizeof(T));
- dynamic_ = nullptr;
- }
- // copy over metadata
- size_ = v.size_;
- capacity_ = v.capacity_;
+ copy_from(v);
}
+
InlinedVector& operator=(const InlinedVector& v) {
if (this != &v) {
clear();
- // if v is allocated, then we copy it's buffer
- if (v.dynamic_ != nullptr) {
- reserve(v.capacity_);
- memcpy(dynamic_, v.dynamic_, v.capacity_ * sizeof(T));
- } else {
- memcpy(inline_, v.inline_, v.capacity_ * sizeof(T));
- dynamic_ = nullptr;
- }
- // copy over metadata
- size_ = v.size_;
- capacity_ = v.capacity_;
+ copy_from(v);
}
return *this;
}
- // move constructors
+ // move constructor
InlinedVector(InlinedVector&& v) {
- // if v is allocated, then we steal it's buffer
- if (v.dynamic_ != nullptr) {
- dynamic_ = v.dynamic_;
- } else {
- memcpy(inline_, v.inline_, v.capacity_ * sizeof(T));
- dynamic_ = nullptr;
- }
- // copy over metadata
- size_ = v.size_;
- capacity_ = v.capacity_;
- // null out the original
- v.dynamic_ = nullptr;
- v.size_ = 0;
- v.capacity_ = 0;
+ init_data();
+ move_from(v);
}
+
InlinedVector& operator=(InlinedVector&& v) {
if (this != &v) {
clear();
- // if v is allocated, then we steal it's buffer
- if (v.dynamic_ != nullptr) {
- dynamic_ = v.dynamic_;
- } else {
- memcpy(inline_, v.inline_, v.capacity_ * sizeof(T));
- dynamic_ = nullptr;
- }
- // copy over metadata
- size_ = v.size_;
- capacity_ = v.capacity_;
- // null out the original
- v.dynamic_ = nullptr;
- v.size_ = 0;
- v.capacity_ = 0;
+ move_from(v);
}
return *this;
}
@@ -166,6 +123,33 @@ class InlinedVector {
void push_back(T&& value) { emplace_back(std::move(value)); }
+ void copy_from(const InlinedVector& v) {
+ // if copy over the buffer from v.
+ if (v.dynamic_ != nullptr) {
+ reserve(v.capacity_);
+ memcpy(dynamic_, v.dynamic_, v.size_ * sizeof(T));
+ } else {
+ memcpy(inline_, v.inline_, v.size_ * sizeof(T));
+ }
+ // copy over metadata
+ size_ = v.size_;
+ capacity_ = v.capacity_;
+ }
+
+ void move_from(InlinedVector& v) {
+ // if v is allocated, then we steal its buffer, else we copy it.
+ if (v.dynamic_ != nullptr) {
+ dynamic_ = v.dynamic_;
+ } else {
+ memcpy(inline_, v.inline_, v.size_ * sizeof(T));
+ }
+ // copy over metadata
+ size_ = v.size_;
+ capacity_ = v.capacity_;
+ // null out the original
+ v.init_data();
+ }
+
size_t size() const { return size_; }
bool empty() const { return size_ == 0; }
diff --git a/test/core/gprpp/inlined_vector_test.cc b/test/core/gprpp/inlined_vector_test.cc
index ba2f4d9e6f2..1ef7da465cc 100644
--- a/test/core/gprpp/inlined_vector_test.cc
+++ b/test/core/gprpp/inlined_vector_test.cc
@@ -27,9 +27,9 @@ namespace testing {
namespace {
template <typename Vector>
-static void FillVector(Vector* v, int len, int offset = 0) {
+static void FillVector(Vector* v, int len, int start = 0) {
for (int i = 0; i < len; i++) {
- v->push_back(i + offset);
+ v->push_back(i + start);
EXPECT_EQ(i + 1UL, v->size());
}
}
@@ -107,69 +107,195 @@ TEST(InlinedVectorTest, ConstIndexOperator) {
const_func(v);
}
-TEST(InlinedVectorTest, CopyConstructorAndAssignment) {
- typedef InlinedVector<int, 8> IntVec8;
- for (size_t len = 0; len < 20; len++) {
- IntVec8 original;
- FillVector(&original, len);
- EXPECT_EQ(len, original.size());
- EXPECT_LE(len, original.capacity());
+TEST(InlinedVectorTest, CopyConstructerInlined) {
+ const size_t kInlinedLength = 8;
+ const size_t kFillSize = kInlinedLength - 1;
+ typedef InlinedVector<int, kInlinedLength> IntVec8;
+ IntVec8 original;
+ FillVector(&original, kFillSize);
+ EXPECT_EQ(kFillSize, original.size());
+ EXPECT_LE(kFillSize, original.capacity());
+ IntVec8 copy_constructed(original);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_constructed[i]);
+ }
+}
+
+TEST(InlinedVectorTest, CopyConstructerAllocated) {
+ const size_t kInlinedLength = 8;
+ const size_t kFillSize = kInlinedLength + 1;
+ typedef InlinedVector<int, kInlinedLength> IntVec8;
+ IntVec8 original;
+ FillVector(&original, kFillSize);
+ EXPECT_EQ(kFillSize, original.size());
+ EXPECT_LE(kFillSize, original.capacity());
+ IntVec8 copy_constructed(original);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_constructed[i]);
+ }
+}
- IntVec8 copy_constructed(original);
+TEST(InlinedVectorTest, CopyAssignementInlined) {
+ const size_t kInlinedLength = 8;
+ const size_t kFillSize = kInlinedLength - 1;
+ typedef InlinedVector<int, kInlinedLength> IntVec8;
+ IntVec8 original;
+ FillVector(&original, kFillSize);
+ EXPECT_EQ(kFillSize, original.size());
+ EXPECT_LE(kFillSize, original.capacity());
+ // copy assigned vector is inlined
+ {
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, kInlinedLength - 1, 99);
+ copy_assigned = original;
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_assigned[i]);
+ }
+ }
+ // copy assigned vector is allocated
+ {
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, kInlinedLength + 1, 99);
+ copy_assigned = original;
for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_TRUE(original[i] == copy_constructed[i]);
+ EXPECT_EQ(original[i], copy_assigned[i]);
}
+ }
+}
- for (size_t start_len = 0; start_len < 20; start_len++) {
- IntVec8 copy_assigned;
- FillVector(©_assigned, start_len, 99); // Add dummy elements
- copy_assigned = original;
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_TRUE(original[i] == copy_assigned[i]);
- }
+TEST(InlinedVectorTest, CopyAssignementAllocated) {
+ const size_t kInlinedLength = 8;
+ const size_t kFillSize = kInlinedLength + 1;
+ typedef InlinedVector<int, kInlinedLength> IntVec8;
+ IntVec8 original;
+ FillVector(&original, kFillSize);
+ EXPECT_EQ(kFillSize, original.size());
+ EXPECT_LE(kFillSize, original.capacity());
+ // copy assigned vector is inlined
+ {
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, kInlinedLength - 1, 99);
+ copy_assigned = original;
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_assigned[i]);
+ }
+ }
+ // copy assigned vector is allocated
+ {
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, kInlinedLength + 1, 99);
+ copy_assigned = original;
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_assigned[i]);
}
}
}
-TEST(InlinedVectorTest, MoveConstructorAndAssignment) {
- typedef InlinedVector<int, 8> IntVec8;
- for (size_t len = 0; len < 20; len++) {
- IntVec8 original;
- const size_t inlined_capacity = original.capacity();
- FillVector(&original, len);
- EXPECT_EQ(len, original.size());
- EXPECT_LE(len, original.capacity());
-
- {
- IntVec8 tmp(original);
- auto* old_data = tmp.data();
- IntVec8 move_constructed(std::move(tmp));
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_TRUE(original[i] == move_constructed[i]);
- }
- if (original.size() > inlined_capacity) {
- // Allocation is moved as a whole, data stays in place.
- EXPECT_TRUE(move_constructed.data() == old_data);
- } else {
- EXPECT_FALSE(move_constructed.data() == old_data);
- }
+TEST(InlinedVectorTest, MoveConstructorInlined) {
+ const size_t kInlinedLength = 8;
+ const size_t kFillSize = kInlinedLength - 1;
+ typedef InlinedVector<int, kInlinedLength> IntVec8;
+ IntVec8 original;
+ FillVector(&original, kFillSize);
+ EXPECT_EQ(kFillSize, original.size());
+ EXPECT_LE(kFillSize, original.capacity());
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ IntVec8 move_constructed(std::move(tmp));
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_constructed[i]);
+ }
+ // original data was inlined so it should have been copied, not moved.
+ EXPECT_NE(move_constructed.data(), old_data);
+}
+
+TEST(InlinedVectorTest, MoveConstructorAllocated) {
+ const size_t kInlinedLength = 8;
+ const size_t kFillSize = kInlinedLength + 1;
+ typedef InlinedVector<int, kInlinedLength> IntVec8;
+ IntVec8 original;
+ FillVector(&original, kFillSize);
+ EXPECT_EQ(kFillSize, original.size());
+ EXPECT_LE(kFillSize, original.capacity());
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ IntVec8 move_constructed(std::move(tmp));
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_constructed[i]);
+ }
+ // original data was allocated, so it should been moved, not copied
+ EXPECT_EQ(move_constructed.data(), old_data);
+}
+
+TEST(InlinedVectorTest, MoveAssignmentInlined) {
+ const size_t kInlinedLength = 8;
+ const size_t kFillSize = kInlinedLength - 1;
+ typedef InlinedVector<int, kInlinedLength> IntVec8;
+ IntVec8 original;
+ FillVector(&original, kFillSize);
+ EXPECT_EQ(kFillSize, original.size());
+ EXPECT_LE(kFillSize, original.capacity());
+ // move assigned vector is inlined
+ {
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, kInlinedLength - 1, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_assigned[i]);
+ }
+ // original data was inlined so it should have been copied, not moved.
+ EXPECT_NE(move_assigned.data(), old_data);
+ }
+ // move assigned vector is allocated
+ {
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, kInlinedLength + 1, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_assigned[i]);
}
- for (size_t start_len = 0; start_len < 20; start_len++) {
- IntVec8 move_assigned;
- FillVector(&move_assigned, start_len, 99); // Add dummy elements
- IntVec8 tmp(original);
- auto* old_data = tmp.data();
- move_assigned = std::move(tmp);
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_TRUE(original[i] == move_assigned[i]);
- }
- if (original.size() > inlined_capacity) {
- // Allocation is moved as a whole, data stays in place.
- EXPECT_TRUE(move_assigned.data() == old_data);
- } else {
- EXPECT_FALSE(move_assigned.data() == old_data);
- }
+ // original data was inlined so it should have been copied, not moved.
+ EXPECT_NE(move_assigned.data(), old_data);
+ }
+}
+
+TEST(InlinedVectorTest, MoveAssignmentAllocated) {
+ const size_t kInlinedLength = 8;
+ const size_t kFillSize = kInlinedLength + 1;
+ typedef InlinedVector<int, kInlinedLength> IntVec8;
+ IntVec8 original;
+ FillVector(&original, kFillSize);
+ EXPECT_EQ(kFillSize, original.size());
+ EXPECT_LE(kFillSize, original.capacity());
+ // move assigned vector is inlined
+ {
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, kInlinedLength - 1, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_assigned[i]);
+ }
+ // original data was allocated so it should have been moved, not copied.
+ EXPECT_EQ(move_assigned.data(), old_data);
+ }
+ // move assigned vector is allocated
+ {
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, kInlinedLength + 1, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_assigned[i]);
}
+ // original data was allocated so it should have been moved, not copied.
+ EXPECT_EQ(move_assigned.data(), old_data);
}
}
From 2968bf687af0f5e0db591d20276b79a7fd627c31 Mon Sep 17 00:00:00 2001
From: ncteisen <ncteisen@gmail.com>
Date: Tue, 17 Jul 2018 15:11:27 -0700
Subject: [PATCH 14/14] reviewer feedback
---
src/core/lib/gprpp/inlined_vector.h | 2 +-
test/core/gprpp/inlined_vector_test.cc | 229 +++++++++++--------------
2 files changed, 98 insertions(+), 133 deletions(-)
diff --git a/src/core/lib/gprpp/inlined_vector.h b/src/core/lib/gprpp/inlined_vector.h
index 508fb2eed11..76e2f0a7850 100644
--- a/src/core/lib/gprpp/inlined_vector.h
+++ b/src/core/lib/gprpp/inlined_vector.h
@@ -124,7 +124,7 @@ class InlinedVector {
void push_back(T&& value) { emplace_back(std::move(value)); }
void copy_from(const InlinedVector& v) {
- // if copy over the buffer from v.
+ // if v is allocated, copy over the buffer.
if (v.dynamic_ != nullptr) {
reserve(v.capacity_);
memcpy(dynamic_, v.dynamic_, v.size_ * sizeof(T));
diff --git a/test/core/gprpp/inlined_vector_test.cc b/test/core/gprpp/inlined_vector_test.cc
index 1ef7da465cc..e9d1eb2c763 100644
--- a/test/core/gprpp/inlined_vector_test.cc
+++ b/test/core/gprpp/inlined_vector_test.cc
@@ -32,6 +32,8 @@ static void FillVector(Vector* v, int len, int start = 0) {
v->push_back(i + start);
EXPECT_EQ(i + 1UL, v->size());
}
+ EXPECT_EQ(static_cast<size_t>(len), v->size());
+ EXPECT_LE(static_cast<size_t>(len), v->capacity());
}
} // namespace
@@ -107,14 +109,16 @@ TEST(InlinedVectorTest, ConstIndexOperator) {
const_func(v);
}
+// the following constants and typedefs are used for copy/move
+// construction/assignment
+const size_t kInlinedLength = 8;
+typedef InlinedVector<int, kInlinedLength> IntVec8;
+const size_t kInlinedFillSize = kInlinedLength - 1;
+const size_t kAllocatedFillSize = kInlinedLength + 1;
+
TEST(InlinedVectorTest, CopyConstructerInlined) {
- const size_t kInlinedLength = 8;
- const size_t kFillSize = kInlinedLength - 1;
- typedef InlinedVector<int, kInlinedLength> IntVec8;
IntVec8 original;
- FillVector(&original, kFillSize);
- EXPECT_EQ(kFillSize, original.size());
- EXPECT_LE(kFillSize, original.capacity());
+ FillVector(&original, kInlinedFillSize);
IntVec8 copy_constructed(original);
for (size_t i = 0; i < original.size(); ++i) {
EXPECT_EQ(original[i], copy_constructed[i]);
@@ -122,83 +126,61 @@ TEST(InlinedVectorTest, CopyConstructerInlined) {
}
TEST(InlinedVectorTest, CopyConstructerAllocated) {
- const size_t kInlinedLength = 8;
- const size_t kFillSize = kInlinedLength + 1;
- typedef InlinedVector<int, kInlinedLength> IntVec8;
IntVec8 original;
- FillVector(&original, kFillSize);
- EXPECT_EQ(kFillSize, original.size());
- EXPECT_LE(kFillSize, original.capacity());
+ FillVector(&original, kAllocatedFillSize);
IntVec8 copy_constructed(original);
for (size_t i = 0; i < original.size(); ++i) {
EXPECT_EQ(original[i], copy_constructed[i]);
}
}
-TEST(InlinedVectorTest, CopyAssignementInlined) {
- const size_t kInlinedLength = 8;
- const size_t kFillSize = kInlinedLength - 1;
- typedef InlinedVector<int, kInlinedLength> IntVec8;
+TEST(InlinedVectorTest, CopyAssignementInlinedInlined) {
IntVec8 original;
- FillVector(&original, kFillSize);
- EXPECT_EQ(kFillSize, original.size());
- EXPECT_LE(kFillSize, original.capacity());
- // copy assigned vector is inlined
- {
- IntVec8 copy_assigned;
- FillVector(©_assigned, kInlinedLength - 1, 99);
- copy_assigned = original;
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_EQ(original[i], copy_assigned[i]);
- }
+ FillVector(&original, kInlinedFillSize);
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, kInlinedFillSize, 99);
+ copy_assigned = original;
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_assigned[i]);
}
- // copy assigned vector is allocated
- {
- IntVec8 copy_assigned;
- FillVector(©_assigned, kInlinedLength + 1, 99);
- copy_assigned = original;
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_EQ(original[i], copy_assigned[i]);
- }
+}
+
+TEST(InlinedVectorTest, CopyAssignementInlinedAllocated) {
+ IntVec8 original;
+ FillVector(&original, kInlinedFillSize);
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, kAllocatedFillSize, 99);
+ copy_assigned = original;
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_assigned[i]);
}
}
-TEST(InlinedVectorTest, CopyAssignementAllocated) {
- const size_t kInlinedLength = 8;
- const size_t kFillSize = kInlinedLength + 1;
- typedef InlinedVector<int, kInlinedLength> IntVec8;
+TEST(InlinedVectorTest, CopyAssignementAllocatedInlined) {
IntVec8 original;
- FillVector(&original, kFillSize);
- EXPECT_EQ(kFillSize, original.size());
- EXPECT_LE(kFillSize, original.capacity());
- // copy assigned vector is inlined
- {
- IntVec8 copy_assigned;
- FillVector(©_assigned, kInlinedLength - 1, 99);
- copy_assigned = original;
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_EQ(original[i], copy_assigned[i]);
- }
+ FillVector(&original, kAllocatedFillSize);
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, kInlinedFillSize, 99);
+ copy_assigned = original;
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_assigned[i]);
}
- // copy assigned vector is allocated
- {
- IntVec8 copy_assigned;
- FillVector(©_assigned, kInlinedLength + 1, 99);
- copy_assigned = original;
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_EQ(original[i], copy_assigned[i]);
- }
+}
+
+TEST(InlinedVectorTest, CopyAssignementAllocatedAllocated) {
+ IntVec8 original;
+ FillVector(&original, kAllocatedFillSize);
+ IntVec8 copy_assigned;
+ FillVector(©_assigned, kAllocatedFillSize, 99);
+ copy_assigned = original;
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], copy_assigned[i]);
}
}
TEST(InlinedVectorTest, MoveConstructorInlined) {
- const size_t kInlinedLength = 8;
- const size_t kFillSize = kInlinedLength - 1;
- typedef InlinedVector<int, kInlinedLength> IntVec8;
IntVec8 original;
- FillVector(&original, kFillSize);
- EXPECT_EQ(kFillSize, original.size());
- EXPECT_LE(kFillSize, original.capacity());
+ FillVector(&original, kInlinedFillSize);
IntVec8 tmp(original);
auto* old_data = tmp.data();
IntVec8 move_constructed(std::move(tmp));
@@ -210,13 +192,8 @@ TEST(InlinedVectorTest, MoveConstructorInlined) {
}
TEST(InlinedVectorTest, MoveConstructorAllocated) {
- const size_t kInlinedLength = 8;
- const size_t kFillSize = kInlinedLength + 1;
- typedef InlinedVector<int, kInlinedLength> IntVec8;
IntVec8 original;
- FillVector(&original, kFillSize);
- EXPECT_EQ(kFillSize, original.size());
- EXPECT_LE(kFillSize, original.capacity());
+ FillVector(&original, kAllocatedFillSize);
IntVec8 tmp(original);
auto* old_data = tmp.data();
IntVec8 move_constructed(std::move(tmp));
@@ -227,76 +204,64 @@ TEST(InlinedVectorTest, MoveConstructorAllocated) {
EXPECT_EQ(move_constructed.data(), old_data);
}
-TEST(InlinedVectorTest, MoveAssignmentInlined) {
- const size_t kInlinedLength = 8;
- const size_t kFillSize = kInlinedLength - 1;
- typedef InlinedVector<int, kInlinedLength> IntVec8;
+TEST(InlinedVectorTest, MoveAssignmentInlinedInlined) {
IntVec8 original;
- FillVector(&original, kFillSize);
- EXPECT_EQ(kFillSize, original.size());
- EXPECT_LE(kFillSize, original.capacity());
- // move assigned vector is inlined
- {
- IntVec8 move_assigned;
- FillVector(&move_assigned, kInlinedLength - 1, 99); // Add dummy elements
- IntVec8 tmp(original);
- auto* old_data = tmp.data();
- move_assigned = std::move(tmp);
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_EQ(original[i], move_assigned[i]);
- }
- // original data was inlined so it should have been copied, not moved.
- EXPECT_NE(move_assigned.data(), old_data);
+ FillVector(&original, kInlinedFillSize);
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, kInlinedFillSize, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_assigned[i]);
}
- // move assigned vector is allocated
- {
- IntVec8 move_assigned;
- FillVector(&move_assigned, kInlinedLength + 1, 99); // Add dummy elements
- IntVec8 tmp(original);
- auto* old_data = tmp.data();
- move_assigned = std::move(tmp);
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_EQ(original[i], move_assigned[i]);
- }
- // original data was inlined so it should have been copied, not moved.
- EXPECT_NE(move_assigned.data(), old_data);
+ // original data was inlined so it should have been copied, not moved.
+ EXPECT_NE(move_assigned.data(), old_data);
+}
+
+TEST(InlinedVectorTest, MoveAssignmentInlinedAllocated) {
+ IntVec8 original;
+ FillVector(&original, kInlinedFillSize);
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, kAllocatedFillSize, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_assigned[i]);
}
+ // original data was inlined so it should have been copied, not moved.
+ EXPECT_NE(move_assigned.data(), old_data);
}
-TEST(InlinedVectorTest, MoveAssignmentAllocated) {
- const size_t kInlinedLength = 8;
- const size_t kFillSize = kInlinedLength + 1;
- typedef InlinedVector<int, kInlinedLength> IntVec8;
+TEST(InlinedVectorTest, MoveAssignmentAllocatedInlined) {
IntVec8 original;
- FillVector(&original, kFillSize);
- EXPECT_EQ(kFillSize, original.size());
- EXPECT_LE(kFillSize, original.capacity());
- // move assigned vector is inlined
- {
- IntVec8 move_assigned;
- FillVector(&move_assigned, kInlinedLength - 1, 99); // Add dummy elements
- IntVec8 tmp(original);
- auto* old_data = tmp.data();
- move_assigned = std::move(tmp);
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_EQ(original[i], move_assigned[i]);
- }
- // original data was allocated so it should have been moved, not copied.
- EXPECT_EQ(move_assigned.data(), old_data);
+ FillVector(&original, kAllocatedFillSize);
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, kInlinedFillSize, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_assigned[i]);
}
- // move assigned vector is allocated
- {
- IntVec8 move_assigned;
- FillVector(&move_assigned, kInlinedLength + 1, 99); // Add dummy elements
- IntVec8 tmp(original);
- auto* old_data = tmp.data();
- move_assigned = std::move(tmp);
- for (size_t i = 0; i < original.size(); ++i) {
- EXPECT_EQ(original[i], move_assigned[i]);
- }
- // original data was allocated so it should have been moved, not copied.
- EXPECT_EQ(move_assigned.data(), old_data);
+ // original data was allocated so it should have been moved, not copied.
+ EXPECT_EQ(move_assigned.data(), old_data);
+}
+
+TEST(InlinedVectorTest, MoveAssignmentAllocatedAllocated) {
+ IntVec8 original;
+ FillVector(&original, kAllocatedFillSize);
+ IntVec8 move_assigned;
+ FillVector(&move_assigned, kAllocatedFillSize, 99); // Add dummy elements
+ IntVec8 tmp(original);
+ auto* old_data = tmp.data();
+ move_assigned = std::move(tmp);
+ for (size_t i = 0; i < original.size(); ++i) {
+ EXPECT_EQ(original[i], move_assigned[i]);
}
+ // original data was allocated so it should have been moved, not copied.
+ EXPECT_EQ(move_assigned.data(), old_data);
}
} // namespace testing