From 4345ef121a3422bd5de4d99bd758e4fd8567680d Mon Sep 17 00:00:00 2001 From: Soheil Hassas Yeganeh Date: Fri, 30 Nov 2018 13:40:12 -0500 Subject: [PATCH] Add debug-only tracing to grpc_core::RefCount Also, this patch removes the *WithTracing variants in favor of the new API. --- .../health/health_check_client.cc | 6 +- .../health/health_check_client.h | 5 +- .../ext/filters/client_channel/lb_policy.cc | 2 +- .../ext/filters/client_channel/lb_policy.h | 3 +- .../client_channel/lb_policy/grpclb/grpclb.cc | 5 +- .../lb_policy/subchannel_list.h | 8 +- .../client_channel/lb_policy/xds/xds.cc | 5 +- .../ext/filters/client_channel/resolver.cc | 2 +- .../ext/filters/client_channel/resolver.h | 2 +- .../ext/filters/client_channel/subchannel.cc | 2 +- .../ext/filters/client_channel/subchannel.h | 2 +- .../chttp2/transport/chttp2_transport.cc | 26 +-- .../ext/transport/chttp2/transport/internal.h | 16 +- src/core/lib/debug/trace.h | 5 +- src/core/lib/gprpp/orphanable.h | 89 ++--------- src/core/lib/gprpp/ref_counted.h | 148 ++++++++---------- src/core/lib/gprpp/ref_counted_ptr.h | 33 ++-- test/core/gprpp/orphanable_test.cc | 4 +- test/core/gprpp/ref_counted_ptr_test.cc | 4 +- test/core/gprpp/ref_counted_test.cc | 9 +- 20 files changed, 141 insertions(+), 235 deletions(-) diff --git a/src/core/ext/filters/client_channel/health/health_check_client.cc b/src/core/ext/filters/client_channel/health/health_check_client.cc index 587919596ff..2232c57120e 100644 --- a/src/core/ext/filters/client_channel/health/health_check_client.cc +++ b/src/core/ext/filters/client_channel/health/health_check_client.cc @@ -51,8 +51,7 @@ HealthCheckClient::HealthCheckClient( RefCountedPtr connected_subchannel, grpc_pollset_set* interested_parties, grpc_core::RefCountedPtr channelz_node) - : InternallyRefCountedWithTracing( - &grpc_health_check_client_trace), + : InternallyRefCounted(&grpc_health_check_client_trace), service_name_(service_name), connected_subchannel_(std::move(connected_subchannel)), interested_parties_(interested_parties), @@ -281,8 +280,7 @@ bool DecodeResponse(grpc_slice_buffer* slice_buffer, grpc_error** error) { HealthCheckClient::CallState::CallState( RefCountedPtr health_check_client, grpc_pollset_set* interested_parties) - : InternallyRefCountedWithTracing( - &grpc_health_check_client_trace), + : InternallyRefCounted(&grpc_health_check_client_trace), health_check_client_(std::move(health_check_client)), pollent_(grpc_polling_entity_create_from_pollset_set(interested_parties)), arena_(gpr_arena_create(health_check_client_->connected_subchannel_ diff --git a/src/core/ext/filters/client_channel/health/health_check_client.h b/src/core/ext/filters/client_channel/health/health_check_client.h index f6babef7d62..2369b73feac 100644 --- a/src/core/ext/filters/client_channel/health/health_check_client.h +++ b/src/core/ext/filters/client_channel/health/health_check_client.h @@ -41,8 +41,7 @@ namespace grpc_core { -class HealthCheckClient - : public InternallyRefCountedWithTracing { +class HealthCheckClient : public InternallyRefCounted { public: HealthCheckClient(const char* service_name, RefCountedPtr connected_subchannel, @@ -61,7 +60,7 @@ class HealthCheckClient private: // Contains a call to the backend and all the data related to the call. - class CallState : public InternallyRefCountedWithTracing { + class CallState : public InternallyRefCounted { public: CallState(RefCountedPtr health_check_client, grpc_pollset_set* interested_parties_); diff --git a/src/core/ext/filters/client_channel/lb_policy.cc b/src/core/ext/filters/client_channel/lb_policy.cc index e065f45639b..b4e803689e9 100644 --- a/src/core/ext/filters/client_channel/lb_policy.cc +++ b/src/core/ext/filters/client_channel/lb_policy.cc @@ -27,7 +27,7 @@ grpc_core::DebugOnlyTraceFlag grpc_trace_lb_policy_refcount( namespace grpc_core { LoadBalancingPolicy::LoadBalancingPolicy(const Args& args) - : InternallyRefCountedWithTracing(&grpc_trace_lb_policy_refcount), + : InternallyRefCounted(&grpc_trace_lb_policy_refcount), combiner_(GRPC_COMBINER_REF(args.combiner, "lb_policy")), client_channel_factory_(args.client_channel_factory), interested_parties_(grpc_pollset_set_create()), diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h index 6733fdca814..7034da6249c 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/src/core/ext/filters/client_channel/lb_policy.h @@ -42,8 +42,7 @@ namespace grpc_core { /// /// Any I/O done by the LB policy should be done under the pollset_set /// returned by \a interested_parties(). -class LoadBalancingPolicy - : public InternallyRefCountedWithTracing { +class LoadBalancingPolicy : public InternallyRefCounted { public: struct Args { /// The combiner under which all LB policy calls will be run. 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 7e70f1c28bd..a46579c7f74 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 @@ -171,8 +171,7 @@ class GrpcLb : public LoadBalancingPolicy { }; /// Contains a call to the LB server and all the data related to the call. - class BalancerCallState - : public InternallyRefCountedWithTracing { + class BalancerCallState : public InternallyRefCounted { public: explicit BalancerCallState( RefCountedPtr parent_grpclb_policy); @@ -499,7 +498,7 @@ grpc_lb_addresses* ProcessServerlist(const grpc_grpclb_serverlist* serverlist) { GrpcLb::BalancerCallState::BalancerCallState( RefCountedPtr parent_grpclb_policy) - : InternallyRefCountedWithTracing(&grpc_lb_glb_trace), + : InternallyRefCounted(&grpc_lb_glb_trace), grpclb_policy_(std::move(parent_grpclb_policy)) { GPR_ASSERT(grpclb_policy_ != nullptr); GPR_ASSERT(!grpclb_policy()->shutting_down_); diff --git a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h index 4ec9e935ed1..f31401502c3 100644 --- a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h +++ b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h @@ -186,8 +186,7 @@ class SubchannelData { // A list of subchannels. template -class SubchannelList - : public InternallyRefCountedWithTracing { +class SubchannelList : public InternallyRefCounted { public: typedef InlinedVector SubchannelVector; @@ -226,8 +225,7 @@ class SubchannelList // Note: Caller must ensure that this is invoked inside of the combiner. void Orphan() override { ShutdownLocked(); - InternallyRefCountedWithTracing::Unref(DEBUG_LOCATION, - "shutdown"); + InternallyRefCounted::Unref(DEBUG_LOCATION, "shutdown"); } GRPC_ABSTRACT_BASE_CLASS @@ -493,7 +491,7 @@ SubchannelList::SubchannelList( const grpc_lb_addresses* addresses, grpc_combiner* combiner, grpc_client_channel_factory* client_channel_factory, const grpc_channel_args& args) - : InternallyRefCountedWithTracing(tracer), + : InternallyRefCounted(tracer), policy_(policy), tracer_(tracer), combiner_(GRPC_COMBINER_REF(combiner, "subchannel_list")) { diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc index 89b86b38a63..563ff42b2ea 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc @@ -166,8 +166,7 @@ class XdsLb : public LoadBalancingPolicy { }; /// Contains a call to the LB server and all the data related to the call. - class BalancerCallState - : public InternallyRefCountedWithTracing { + class BalancerCallState : public InternallyRefCounted { public: explicit BalancerCallState( RefCountedPtr parent_xdslb_policy); @@ -488,7 +487,7 @@ grpc_lb_addresses* ProcessServerlist(const xds_grpclb_serverlist* serverlist) { XdsLb::BalancerCallState::BalancerCallState( RefCountedPtr parent_xdslb_policy) - : InternallyRefCountedWithTracing(&grpc_lb_xds_trace), + : InternallyRefCounted(&grpc_lb_xds_trace), xdslb_policy_(std::move(parent_xdslb_policy)) { GPR_ASSERT(xdslb_policy_ != nullptr); GPR_ASSERT(!xdslb_policy()->shutting_down_); diff --git a/src/core/ext/filters/client_channel/resolver.cc b/src/core/ext/filters/client_channel/resolver.cc index cd11eeb9e4d..601b08be246 100644 --- a/src/core/ext/filters/client_channel/resolver.cc +++ b/src/core/ext/filters/client_channel/resolver.cc @@ -27,7 +27,7 @@ grpc_core::DebugOnlyTraceFlag grpc_trace_resolver_refcount(false, namespace grpc_core { Resolver::Resolver(grpc_combiner* combiner) - : InternallyRefCountedWithTracing(&grpc_trace_resolver_refcount), + : InternallyRefCounted(&grpc_trace_resolver_refcount), combiner_(GRPC_COMBINER_REF(combiner, "resolver")) {} Resolver::~Resolver() { GRPC_COMBINER_UNREF(combiner_, "resolver"); } diff --git a/src/core/ext/filters/client_channel/resolver.h b/src/core/ext/filters/client_channel/resolver.h index e9acbb7c41c..9da849a1017 100644 --- a/src/core/ext/filters/client_channel/resolver.h +++ b/src/core/ext/filters/client_channel/resolver.h @@ -44,7 +44,7 @@ namespace grpc_core { /// /// Note: All methods with a "Locked" suffix must be called from the /// combiner passed to the constructor. -class Resolver : public InternallyRefCountedWithTracing { +class Resolver : public InternallyRefCounted { public: // Not copyable nor movable. Resolver(const Resolver&) = delete; diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index a56db0201b8..0817b1dd392 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -1072,7 +1072,7 @@ ConnectedSubchannel::ConnectedSubchannel( grpc_core::RefCountedPtr channelz_subchannel, intptr_t socket_uuid) - : RefCountedWithTracing(&grpc_trace_stream_refcount), + : RefCounted(&grpc_trace_stream_refcount), channel_stack_(channel_stack), channelz_subchannel_(std::move(channelz_subchannel)), socket_uuid_(socket_uuid) {} diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h index ec3b4d86e41..69c2456ec20 100644 --- a/src/core/ext/filters/client_channel/subchannel.h +++ b/src/core/ext/filters/client_channel/subchannel.h @@ -72,7 +72,7 @@ typedef struct grpc_subchannel_key grpc_subchannel_key; namespace grpc_core { -class ConnectedSubchannel : public RefCountedWithTracing { +class ConnectedSubchannel : public RefCounted { public: struct CallArgs { grpc_polling_entity* pollent; diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 99c675f503a..7377287e8cb 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -207,29 +207,6 @@ grpc_chttp2_transport::~grpc_chttp2_transport() { gpr_free(peer_string); } -#ifndef NDEBUG -void grpc_chttp2_unref_transport(grpc_chttp2_transport* t, const char* reason, - const char* file, int line) { - if (grpc_trace_chttp2_refcount.enabled()) { - const grpc_core::RefCount::Value val = t->refs.get(); - gpr_log(GPR_DEBUG, "chttp2:unref:%p %" PRIdPTR "->%" PRIdPTR " %s [%s:%d]", - t, val, val - 1, reason, file, line); - } - if (!t->refs.Unref()) return; - grpc_core::Delete(t); -} - -void grpc_chttp2_ref_transport(grpc_chttp2_transport* t, const char* reason, - const char* file, int line) { - if (grpc_trace_chttp2_refcount.enabled()) { - const grpc_core::RefCount::Value val = t->refs.get(); - gpr_log(GPR_DEBUG, "chttp2: ref:%p %" PRIdPTR "->%" PRIdPTR " %s [%s:%d]", - t, val, val + 1, reason, file, line); - } - t->refs.Ref(); -} -#endif - static const grpc_transport_vtable* get_vtable(void); /* Returns whether bdp is enabled */ @@ -481,7 +458,8 @@ static void init_keepalive_pings_if_enabled(grpc_chttp2_transport* t) { grpc_chttp2_transport::grpc_chttp2_transport( const grpc_channel_args* channel_args, grpc_endpoint* ep, bool is_client, grpc_resource_user* resource_user) - : ep(ep), + : refs(1, &grpc_trace_chttp2_refcount), + ep(ep), peer_string(grpc_endpoint_get_peer(ep)), resource_user(resource_user), combiner(grpc_combiner_create()), diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 6aa68f5d4a9..341f5b3977f 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -792,10 +792,18 @@ void grpc_chttp2_stream_unref(grpc_chttp2_stream* s); grpc_chttp2_ref_transport(t, r, __FILE__, __LINE__) #define GRPC_CHTTP2_UNREF_TRANSPORT(t, r) \ grpc_chttp2_unref_transport(t, r, __FILE__, __LINE__) -void grpc_chttp2_unref_transport(grpc_chttp2_transport* t, const char* reason, - const char* file, int line); -void grpc_chttp2_ref_transport(grpc_chttp2_transport* t, const char* reason, - const char* file, int line); +inline void grpc_chttp2_unref_transport(grpc_chttp2_transport* t, + const char* reason, const char* file, + int line) { + if (t->refs.Unref(grpc_core::DebugLocation(file, line), reason)) { + grpc_core::Delete(t); + } +} +inline void grpc_chttp2_ref_transport(grpc_chttp2_transport* t, + const char* reason, const char* file, + int line) { + t->refs.Ref(grpc_core::DebugLocation(file, line), reason); +} #else #define GRPC_CHTTP2_REF_TRANSPORT(t, r) grpc_chttp2_ref_transport(t) #define GRPC_CHTTP2_UNREF_TRANSPORT(t, r) grpc_chttp2_unref_transport(t) diff --git a/src/core/lib/debug/trace.h b/src/core/lib/debug/trace.h index fe6301a3fcd..5ed52454bd7 100644 --- a/src/core/lib/debug/trace.h +++ b/src/core/lib/debug/trace.h @@ -102,8 +102,9 @@ typedef TraceFlag DebugOnlyTraceFlag; #else class DebugOnlyTraceFlag { public: - DebugOnlyTraceFlag(bool default_enabled, const char* name) {} - bool enabled() { return false; } + constexpr DebugOnlyTraceFlag(bool default_enabled, const char* name) {} + constexpr bool enabled() const { return false; } + constexpr const char* name() const { return "DebugOnlyTraceFlag"; } private: void set_enabled(bool enabled) {} diff --git a/src/core/lib/gprpp/orphanable.h b/src/core/lib/gprpp/orphanable.h index 3eb510165e2..9053c60111f 100644 --- a/src/core/lib/gprpp/orphanable.h +++ b/src/core/lib/gprpp/orphanable.h @@ -90,104 +90,41 @@ class InternallyRefCounted : public Orphanable { template friend class RefCountedPtr; - InternallyRefCounted() = default; + // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag. + // Note: RefCount tracing is only enabled on debug builds, even when a + // TraceFlag is used. + template + explicit InternallyRefCounted(TraceFlagT* trace_flag = nullptr) + : refs_(1, trace_flag) {} virtual ~InternallyRefCounted() = default; RefCountedPtr Ref() GRPC_MUST_USE_RESULT { IncrementRefCount(); return RefCountedPtr(static_cast(this)); } - - void Unref() { - if (refs_.Unref()) { - Delete(static_cast(this)); - } - } - - private: - void IncrementRefCount() { refs_.Ref(); } - - grpc_core::RefCount refs_; -}; - -// An alternative version of the InternallyRefCounted base class that -// supports tracing. This is intended to be used in cases where the -// object will be handled both by idiomatic C++ code using smart -// pointers and legacy code that is manually calling Ref() and Unref(). -// Once all of our code is converted to idiomatic C++, we may be able to -// eliminate this class. -template -class InternallyRefCountedWithTracing : public Orphanable { - public: - // Not copyable nor movable. - InternallyRefCountedWithTracing(const InternallyRefCountedWithTracing&) = - delete; - InternallyRefCountedWithTracing& operator=( - const InternallyRefCountedWithTracing&) = delete; - - GRPC_ABSTRACT_BASE_CLASS - - protected: - GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - - // Allow RefCountedPtr<> to access Unref() and IncrementRefCount(). - template - friend class RefCountedPtr; - - InternallyRefCountedWithTracing() - : InternallyRefCountedWithTracing(static_cast(nullptr)) {} - - explicit InternallyRefCountedWithTracing(TraceFlag* trace_flag) - : trace_flag_(trace_flag) {} - -#ifdef NDEBUG - explicit InternallyRefCountedWithTracing(DebugOnlyTraceFlag* trace_flag) - : InternallyRefCountedWithTracing() {} -#endif - - virtual ~InternallyRefCountedWithTracing() = default; - - RefCountedPtr Ref() GRPC_MUST_USE_RESULT { - IncrementRefCount(); - return RefCountedPtr(static_cast(this)); - } - RefCountedPtr Ref(const DebugLocation& location, const char* reason) GRPC_MUST_USE_RESULT { - if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) { - const grpc_core::RefCount::Value old_refs = refs_.get(); - gpr_log(GPR_INFO, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s", - trace_flag_->name(), this, location.file(), location.line(), - old_refs, old_refs + 1, reason); - } - return Ref(); + IncrementRefCount(location, reason); + return RefCountedPtr(static_cast(this)); } - // TODO(roth): Once all of our code is converted to C++ and can use - // RefCountedPtr<> instead of manual ref-counting, make the Unref() methods - // private, since they will only be used by RefCountedPtr<>, which is a - // friend of this class. - void Unref() { if (refs_.Unref()) { Delete(static_cast(this)); } } - void Unref(const DebugLocation& location, const char* reason) { - if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) { - const grpc_core::RefCount::Value old_refs = refs_.get(); - gpr_log(GPR_INFO, "%s:%p %s:%d unref %" PRIdPTR " -> %" PRIdPTR " %s", - trace_flag_->name(), this, location.file(), location.line(), - old_refs, old_refs - 1, reason); + if (refs_.Unref(location, reason)) { + Delete(static_cast(this)); } - Unref(); } private: void IncrementRefCount() { refs_.Ref(); } + void IncrementRefCount(const DebugLocation& location, const char* reason) { + refs_.Ref(location, reason); + } - TraceFlag* trace_flag_ = nullptr; grpc_core::RefCount refs_; }; diff --git a/src/core/lib/gprpp/ref_counted.h b/src/core/lib/gprpp/ref_counted.h index 98de1a3653f..fa97ffcfed2 100644 --- a/src/core/lib/gprpp/ref_counted.h +++ b/src/core/lib/gprpp/ref_counted.h @@ -74,12 +74,34 @@ class RefCount { using Value = intptr_t; // `init` is the initial refcount stored in this object. - constexpr explicit RefCount(Value init = 1) : value_(init) {} + // + // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag. + // Note: RefCount tracing is only enabled on debug builds, even when a + // TraceFlag is used. + template + constexpr explicit RefCount(Value init = 1, TraceFlagT* trace_flag = nullptr) + : +#ifndef NDEBUG + trace_flag_(trace_flag), +#endif + value_(init) { + } // Increases the ref-count by `n`. void Ref(Value n = 1) { GPR_ATM_INC_ADD_THEN(value_.fetch_add(n, std::memory_order_relaxed)); } + void Ref(const DebugLocation& location, const char* reason, Value n = 1) { +#ifndef NDEBUG + if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) { + const RefCount::Value old_refs = get(); + gpr_log(GPR_INFO, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s", + trace_flag_->name(), this, location.file(), location.line(), + old_refs, old_refs + n, reason); + } +#endif + Ref(n); + } // Similar to Ref() with an assert on the ref-count being non-zero. void RefNonZero() { @@ -91,6 +113,17 @@ class RefCount { Ref(); #endif } + void RefNonZero(const DebugLocation& location, const char* reason) { +#ifndef NDEBUG + if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) { + const RefCount::Value old_refs = get(); + gpr_log(GPR_INFO, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s", + trace_flag_->name(), this, location.file(), location.line(), + old_refs, old_refs + 1, reason); + } +#endif + RefNonZero(); + } // Decrements the ref-count and returns true if the ref-count reaches 0. bool Unref() { @@ -99,10 +132,24 @@ class RefCount { GPR_DEBUG_ASSERT(prior > 0); return prior == 1; } + bool Unref(const DebugLocation& location, const char* reason) { +#ifndef NDEBUG + if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) { + const RefCount::Value old_refs = get(); + gpr_log(GPR_INFO, "%s:%p %s:%d unref %" PRIdPTR " -> %" PRIdPTR " %s", + trace_flag_->name(), this, location.file(), location.line(), + old_refs, old_refs - 1, reason); + } +#endif + return Unref(); + } + private: Value get() const { return value_.load(std::memory_order_relaxed); } - private: +#ifndef NDEBUG + TraceFlag* trace_flag_; +#endif std::atomic value_; }; @@ -134,54 +181,6 @@ class RefCount { // template class RefCounted : public Impl { - public: - RefCountedPtr Ref() GRPC_MUST_USE_RESULT { - IncrementRefCount(); - return RefCountedPtr(static_cast(this)); - } - - // TODO(roth): Once all of our code is converted to C++ and can use - // RefCountedPtr<> instead of manual ref-counting, make this method - // private, since it will only be used by RefCountedPtr<>, which is a - // friend of this class. - void Unref() { - if (refs_.Unref()) { - Delete(static_cast(this)); - } - } - - // Not copyable nor movable. - RefCounted(const RefCounted&) = delete; - RefCounted& operator=(const RefCounted&) = delete; - - GRPC_ABSTRACT_BASE_CLASS - - protected: - GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - - RefCounted() = default; - - // Note: Depending on the Impl used, this dtor can be implicitly virtual. - ~RefCounted() = default; - - private: - // Allow RefCountedPtr<> to access IncrementRefCount(). - template - friend class RefCountedPtr; - - void IncrementRefCount() { refs_.Ref(); } - - RefCount refs_; -}; - -// An alternative version of the RefCounted base class that -// supports tracing. This is intended to be used in cases where the -// object will be handled both by idiomatic C++ code using smart -// pointers and legacy code that is manually calling Ref() and Unref(). -// Once all of our code is converted to idiomatic C++, we may be able to -// eliminate this class. -template -class RefCountedWithTracing : public Impl { public: RefCountedPtr Ref() GRPC_MUST_USE_RESULT { IncrementRefCount(); @@ -190,58 +189,43 @@ class RefCountedWithTracing : public Impl { RefCountedPtr Ref(const DebugLocation& location, const char* reason) GRPC_MUST_USE_RESULT { - if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) { - const RefCount::Value old_refs = refs_.get(); - gpr_log(GPR_INFO, "%s:%p %s:%d ref %" PRIdPTR " -> %" PRIdPTR " %s", - trace_flag_->name(), this, location.file(), location.line(), - old_refs, old_refs + 1, reason); - } - return Ref(); + IncrementRefCount(location, reason); + return RefCountedPtr(static_cast(this)); } // TODO(roth): Once all of our code is converted to C++ and can use - // RefCountedPtr<> instead of manual ref-counting, make the Unref() methods - // private, since they will only be used by RefCountedPtr<>, which is a + // RefCountedPtr<> instead of manual ref-counting, make this method + // private, since it will only be used by RefCountedPtr<>, which is a // friend of this class. - void Unref() { if (refs_.Unref()) { Delete(static_cast(this)); } } - void Unref(const DebugLocation& location, const char* reason) { - if (location.Log() && trace_flag_ != nullptr && trace_flag_->enabled()) { - const RefCount::Value old_refs = refs_.get(); - gpr_log(GPR_INFO, "%s:%p %s:%d unref %" PRIdPTR " -> %" PRIdPTR " %s", - trace_flag_->name(), this, location.file(), location.line(), - old_refs, old_refs - 1, reason); + if (refs_.Unref(location, reason)) { + Delete(static_cast(this)); } - Unref(); } // Not copyable nor movable. - RefCountedWithTracing(const RefCountedWithTracing&) = delete; - RefCountedWithTracing& operator=(const RefCountedWithTracing&) = delete; + RefCounted(const RefCounted&) = delete; + RefCounted& operator=(const RefCounted&) = delete; GRPC_ABSTRACT_BASE_CLASS protected: GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - RefCountedWithTracing() - : RefCountedWithTracing(static_cast(nullptr)) {} - - explicit RefCountedWithTracing(TraceFlag* trace_flag) - : trace_flag_(trace_flag) {} - -#ifdef NDEBUG - explicit RefCountedWithTracing(DebugOnlyTraceFlag* trace_flag) - : RefCountedWithTracing() {} -#endif + // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag. + // Note: RefCount tracing is only enabled on debug builds, even when a + // TraceFlag is used. + template + explicit RefCounted(TraceFlagT* trace_flag = nullptr) + : refs_(1, trace_flag) {} // Note: Depending on the Impl used, this dtor can be implicitly virtual. - ~RefCountedWithTracing() = default; + ~RefCounted() = default; private: // Allow RefCountedPtr<> to access IncrementRefCount(). @@ -249,8 +233,10 @@ class RefCountedWithTracing : public Impl { friend class RefCountedPtr; void IncrementRefCount() { refs_.Ref(); } + void IncrementRefCount(const DebugLocation& location, const char* reason) { + refs_.Ref(location, reason); + } - TraceFlag* trace_flag_ = nullptr; RefCount refs_; }; diff --git a/src/core/lib/gprpp/ref_counted_ptr.h b/src/core/lib/gprpp/ref_counted_ptr.h index facd7c6dce6..1ed5d584c70 100644 --- a/src/core/lib/gprpp/ref_counted_ptr.h +++ b/src/core/lib/gprpp/ref_counted_ptr.h @@ -24,6 +24,7 @@ #include #include +#include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/memory.h" namespace grpc_core { @@ -55,15 +56,13 @@ class RefCountedPtr { // Move assignment. RefCountedPtr& operator=(RefCountedPtr&& other) { - if (value_ != nullptr) value_->Unref(); - value_ = other.value_; + reset(other.value_); other.value_ = nullptr; return *this; } template RefCountedPtr& operator=(RefCountedPtr&& other) { - if (value_ != nullptr) value_->Unref(); - value_ = other.value_; + reset(other.value_); other.value_ = nullptr; return *this; } @@ -86,8 +85,7 @@ class RefCountedPtr { // Note: Order of reffing and unreffing is important here in case value_ // and other.value_ are the same object. if (other.value_ != nullptr) other.value_->IncrementRefCount(); - if (value_ != nullptr) value_->Unref(); - value_ = other.value_; + reset(other.value_); return *this; } template @@ -97,8 +95,7 @@ class RefCountedPtr { // Note: Order of reffing and unreffing is important here in case value_ // and other.value_ are the same object. if (other.value_ != nullptr) other.value_->IncrementRefCount(); - if (value_ != nullptr) value_->Unref(); - value_ = other.value_; + reset(other.value_); return *this; } @@ -107,21 +104,29 @@ class RefCountedPtr { } // If value is non-null, we take ownership of a ref to it. - void reset(T* value) { + void reset(T* value = nullptr) { if (value_ != nullptr) value_->Unref(); value_ = value; } + void reset(const DebugLocation& location, const char* reason, + T* value = nullptr) { + if (value_ != nullptr) value_->Unref(location, reason); + value_ = value; + } template - void reset(Y* value) { + void reset(Y* value = nullptr) { static_assert(std::has_virtual_destructor::value, "T does not have a virtual dtor"); if (value_ != nullptr) value_->Unref(); value_ = value; } - - void reset() { - if (value_ != nullptr) value_->Unref(); - value_ = nullptr; + template + void reset(const DebugLocation& location, const char* reason, + Y* value = nullptr) { + static_assert(std::has_virtual_destructor::value, + "T does not have a virtual dtor"); + if (value_ != nullptr) value_->Unref(location, reason); + value_ = value; } // TODO(roth): This method exists solely as a transition mechanism to allow diff --git a/test/core/gprpp/orphanable_test.cc b/test/core/gprpp/orphanable_test.cc index 546f395cef6..fe13df0d0d0 100644 --- a/test/core/gprpp/orphanable_test.cc +++ b/test/core/gprpp/orphanable_test.cc @@ -83,11 +83,11 @@ TEST(OrphanablePtr, InternallyRefCounted) { // things build properly in both debug and non-debug cases. DebugOnlyTraceFlag baz_tracer(true, "baz"); -class Baz : public InternallyRefCountedWithTracing { +class Baz : public InternallyRefCounted { public: Baz() : Baz(0) {} explicit Baz(int value) - : InternallyRefCountedWithTracing(&baz_tracer), value_(value) {} + : InternallyRefCounted(&baz_tracer), value_(value) {} void Orphan() override { Unref(); } int value() const { return value_; } diff --git a/test/core/gprpp/ref_counted_ptr_test.cc b/test/core/gprpp/ref_counted_ptr_test.cc index 512687eb679..96dbdf884b0 100644 --- a/test/core/gprpp/ref_counted_ptr_test.cc +++ b/test/core/gprpp/ref_counted_ptr_test.cc @@ -163,9 +163,9 @@ TEST(MakeRefCounted, Args) { TraceFlag foo_tracer(true, "foo"); -class FooWithTracing : public RefCountedWithTracing { +class FooWithTracing : public RefCounted { public: - FooWithTracing() : RefCountedWithTracing(&foo_tracer) {} + FooWithTracing() : RefCounted(&foo_tracer) {} }; TEST(RefCountedPtr, RefCountedWithTracing) { diff --git a/test/core/gprpp/ref_counted_test.cc b/test/core/gprpp/ref_counted_test.cc index 5aba1634efb..1955be33115 100644 --- a/test/core/gprpp/ref_counted_test.cc +++ b/test/core/gprpp/ref_counted_test.cc @@ -74,9 +74,9 @@ TEST(RefCountedNonPolymorphic, ExtraRef) { // things build properly in both debug and non-debug cases. DebugOnlyTraceFlag foo_tracer(true, "foo"); -class FooWithTracing : public RefCountedWithTracing { +class FooWithTracing : public RefCounted { public: - FooWithTracing() : RefCountedWithTracing(&foo_tracer) {} + FooWithTracing() : RefCounted(&foo_tracer) {} }; TEST(RefCountedWithTracing, Basic) { @@ -92,10 +92,9 @@ TEST(RefCountedWithTracing, Basic) { } class FooNonPolymorphicWithTracing - : public RefCountedWithTracing { + : public RefCounted { public: - FooNonPolymorphicWithTracing() : RefCountedWithTracing(&foo_tracer) {} + FooNonPolymorphicWithTracing() : RefCounted(&foo_tracer) {} }; TEST(RefCountedNonPolymorphicWithTracing, Basic) {