From 4fc906a5fe9ff8a39edf013b4af487010317eed4 Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Thu, 3 Oct 2019 10:50:55 -0700 Subject: [PATCH 1/2] Clean up memory code --- .../client_channel/lb_policy/grpclb/grpclb.cc | 5 +---- src/core/ext/filters/client_channel/resolver.h | 5 +---- .../filters/client_channel/retry_throttle.h | 5 +---- src/core/lib/gprpp/README.md | 6 +----- src/core/lib/gprpp/memory.h | 18 ------------------ src/core/lib/gprpp/orphanable.h | 2 -- src/core/lib/gprpp/ref_counted.h | 16 +++++----------- src/core/lib/iomgr/buffer_list.h | 4 +--- src/core/lib/iomgr/cfstream_handle.h | 3 --- src/core/lib/slice/slice.cc | 11 ++--------- src/core/lib/slice/slice_hash_table.h | 5 +---- src/core/lib/slice/slice_weak_hash_table.h | 9 +++------ .../tsi/ssl/session_cache/ssl_session_cache.h | 9 +++------ 13 files changed, 19 insertions(+), 79 deletions(-) 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 20c6c1af601..54b3dd457f7 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 @@ -151,6 +151,7 @@ class GrpcLb : public LoadBalancingPolicy { public: explicit BalancerCallState( RefCountedPtr parent_grpclb_policy); + ~BalancerCallState(); // It's the caller's responsibility to ensure that Orphan() is called from // inside the combiner. @@ -164,10 +165,6 @@ class GrpcLb : public LoadBalancingPolicy { bool seen_serverlist() const { return seen_serverlist_; } private: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - - ~BalancerCallState(); - GrpcLb* grpclb_policy() const { return static_cast(grpclb_policy_.get()); } diff --git a/src/core/ext/filters/client_channel/resolver.h b/src/core/ext/filters/client_channel/resolver.h index 609d0a7a233..ebeb3912032 100644 --- a/src/core/ext/filters/client_channel/resolver.h +++ b/src/core/ext/filters/client_channel/resolver.h @@ -87,6 +87,7 @@ class Resolver : public InternallyRefCounted { // Not copyable nor movable. Resolver(const Resolver&) = delete; Resolver& operator=(const Resolver&) = delete; + virtual ~Resolver(); /// Starts resolving. virtual void StartLocked() = 0; @@ -121,8 +122,6 @@ class Resolver : public InternallyRefCounted { } protected: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - /// Does NOT take ownership of the reference to \a combiner. // TODO(roth): Once we have a C++-like interface for combiners, this // API should change to take a RefCountedPtr<>, so that we always take @@ -130,8 +129,6 @@ class Resolver : public InternallyRefCounted { explicit Resolver(grpc_combiner* combiner, UniquePtr result_handler); - virtual ~Resolver(); - /// Shuts down the resolver. virtual void ShutdownLocked() = 0; diff --git a/src/core/ext/filters/client_channel/retry_throttle.h b/src/core/ext/filters/client_channel/retry_throttle.h index 9e2fff56d24..897a617f59a 100644 --- a/src/core/ext/filters/client_channel/retry_throttle.h +++ b/src/core/ext/filters/client_channel/retry_throttle.h @@ -32,6 +32,7 @@ class ServerRetryThrottleData : public RefCounted { public: ServerRetryThrottleData(intptr_t max_milli_tokens, intptr_t milli_token_ratio, ServerRetryThrottleData* old_throttle_data); + ~ServerRetryThrottleData(); /// Records a failure. Returns true if it's okay to send a retry. bool RecordFailure(); @@ -43,10 +44,6 @@ class ServerRetryThrottleData : public RefCounted { intptr_t milli_token_ratio() const { return milli_token_ratio_; } private: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - - ~ServerRetryThrottleData(); - void GetReplacementThrottleDataIfNeeded( ServerRetryThrottleData** throttle_data); diff --git a/src/core/lib/gprpp/README.md b/src/core/lib/gprpp/README.md index eab018bb312..f541d22bbcd 100644 --- a/src/core/lib/gprpp/README.md +++ b/src/core/lib/gprpp/README.md @@ -9,8 +9,4 @@ the use of portability macros. Note that this is the only place in src/core where we allow use of the C++ standard library (i.e., anything in the `std::` -namespace). And for now, we require that any use of the -standard library is build-time-only -- i.e., we do not allow -run-time dependencies on libstdc++. For more details, see -[gRFC L6](/grpc/proposal/blob/master/L6-allow-c%2B%2B-in-grpc-core.md) and -[Moving gRPC core to C++](/grpc/grpc/blob/master/doc/core/moving-to-c%2B%2B.md). +namespace). diff --git a/src/core/lib/gprpp/memory.h b/src/core/lib/gprpp/memory.h index c1478f2aa73..149db3858aa 100644 --- a/src/core/lib/gprpp/memory.h +++ b/src/core/lib/gprpp/memory.h @@ -28,24 +28,6 @@ #include #include -// Add this to a class that want to use Delete(), but has a private or -// protected destructor. -// Should not be used in new code. -// TODO(juanlishen): Remove this macro, and instead comment that the public dtor -// should not be used directly. -#define GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE \ - template \ - friend void ::grpc_core::Delete(_Delete_T*); - -// Add this to a class that want to use New(), but has a private or -// protected constructor. -// Should not be used in new code. -// TODO(juanlishen): Remove this macro, and instead comment that the public dtor -// should not be used directly. -#define GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW \ - template \ - friend _New_T* grpc_core::New(_New_Args&&...); - namespace grpc_core { // Alternative to new, to ensure memory allocation being wrapped to gpr_malloc diff --git a/src/core/lib/gprpp/orphanable.h b/src/core/lib/gprpp/orphanable.h index 03511e8a4db..db9547d2224 100644 --- a/src/core/lib/gprpp/orphanable.h +++ b/src/core/lib/gprpp/orphanable.h @@ -81,8 +81,6 @@ class InternallyRefCounted : public Orphanable { InternallyRefCounted& operator=(const InternallyRefCounted&) = delete; protected: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - // Allow RefCountedPtr<> to access Unref() and IncrementRefCount(). template friend class RefCountedPtr; diff --git a/src/core/lib/gprpp/ref_counted.h b/src/core/lib/gprpp/ref_counted.h index 086156f2d74..ff0d3a03527 100644 --- a/src/core/lib/gprpp/ref_counted.h +++ b/src/core/lib/gprpp/ref_counted.h @@ -39,9 +39,7 @@ namespace grpc_core { // PolymorphicRefCount enforces polymorphic destruction of RefCounted. class PolymorphicRefCount { - protected: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - + public: virtual ~PolymorphicRefCount() = default; }; @@ -49,9 +47,7 @@ class PolymorphicRefCount { // RefCounted. Please refer to grpc_core::RefCounted for more details, and // when in doubt use PolymorphicRefCount. class NonPolymorphicRefCount { - protected: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - + public: ~NonPolymorphicRefCount() = default; }; @@ -230,6 +226,9 @@ class RefCount { template class RefCounted : public Impl { public: + // Note: Depending on the Impl used, this dtor can be implicitly virtual. + ~RefCounted() = default; + RefCountedPtr Ref() GRPC_MUST_USE_RESULT { IncrementRefCount(); return RefCountedPtr(static_cast(this)); @@ -266,8 +265,6 @@ class RefCounted : public Impl { RefCounted& operator=(const RefCounted&) = delete; protected: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - // TraceFlagT is defined to accept both DebugOnlyTraceFlag and TraceFlag. // Note: RefCount tracing is only enabled on debug builds, even when a // TraceFlag is used. @@ -276,9 +273,6 @@ class RefCounted : public Impl { intptr_t initial_refcount = 1) : refs_(initial_refcount, trace_flag) {} - // Note: Depending on the Impl used, this dtor can be implicitly virtual. - ~RefCounted() = default; - private: // Allow RefCountedPtr<> to access IncrementRefCount(). template diff --git a/src/core/lib/iomgr/buffer_list.h b/src/core/lib/iomgr/buffer_list.h index 755ce02ba95..1e851dc4c9d 100644 --- a/src/core/lib/iomgr/buffer_list.h +++ b/src/core/lib/iomgr/buffer_list.h @@ -132,12 +132,10 @@ class TracedBuffer { static void Shutdown(grpc_core::TracedBuffer** head, void* remaining, grpc_error* shutdown_err); - private: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW - TracedBuffer(uint32_t seq_no, void* arg) : seq_no_(seq_no), arg_(arg), next_(nullptr) {} + private: uint32_t seq_no_; /* The sequence number for the last byte in the buffer */ void* arg_; /* The arg to pass to timestamps_callback */ grpc_core::Timestamps ts_; /* The timestamps corresponding to this buffer */ diff --git a/src/core/lib/iomgr/cfstream_handle.h b/src/core/lib/iomgr/cfstream_handle.h index 5f3a525930c..96765b89037 100644 --- a/src/core/lib/iomgr/cfstream_handle.h +++ b/src/core/lib/iomgr/cfstream_handle.h @@ -72,9 +72,6 @@ class CFStreamHandle : public GrpcLibraryInitHolder { dispatch_queue_t dispatch_queue_; gpr_refcount refcount_; - - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE }; #ifdef DEBUG diff --git a/src/core/lib/slice/slice.cc b/src/core/lib/slice/slice.cc index d3480abaa87..5cafb4a6f3f 100644 --- a/src/core/lib/slice/slice.cc +++ b/src/core/lib/slice/slice.cc @@ -82,13 +82,11 @@ class NewSliceRefcount { &base_), user_destroy_(destroy), user_data_(user_data) {} - - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE + ~NewSliceRefcount() { user_destroy_(user_data_); } grpc_slice_refcount* base_refcount() { return &base_; } private: - ~NewSliceRefcount() { user_destroy_(user_data_); } grpc_slice_refcount base_; RefCount refs_; @@ -147,14 +145,11 @@ class NewWithLenSliceRefcount { user_data_(user_data), user_length_(user_length), user_destroy_(destroy) {} - - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE + ~NewWithLenSliceRefcount() { user_destroy_(user_data_, user_length_); } grpc_slice_refcount* base_refcount() { return &base_; } private: - ~NewWithLenSliceRefcount() { user_destroy_(user_data_, user_length_); } - grpc_slice_refcount base_; RefCount refs_; void* user_data_; @@ -170,8 +165,6 @@ class MovedStringSliceRefCount { &base_), str_(std::move(str)) {} - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - grpc_slice_refcount* base_refcount() { return &base_; } private: diff --git a/src/core/lib/slice/slice_hash_table.h b/src/core/lib/slice/slice_hash_table.h index bd003c2c5b4..f94fd08cad0 100644 --- a/src/core/lib/slice/slice_hash_table.h +++ b/src/core/lib/slice/slice_hash_table.h @@ -77,13 +77,10 @@ class SliceHashTable : public RefCounted> { /// - else, if value_cmp(a_value, b_value) < 1 (resp. > 1). static int Cmp(const SliceHashTable& a, const SliceHashTable& b); - private: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW - SliceHashTable(size_t num_entries, Entry* entries, ValueCmp value_cmp); virtual ~SliceHashTable(); + private: void Add(const grpc_slice& key, T& value); // Default value comparison function, if none specified by caller. diff --git a/src/core/lib/slice/slice_weak_hash_table.h b/src/core/lib/slice/slice_weak_hash_table.h index bd59a4c2e48..0d691566033 100644 --- a/src/core/lib/slice/slice_weak_hash_table.h +++ b/src/core/lib/slice/slice_weak_hash_table.h @@ -39,6 +39,9 @@ namespace grpc_core { template class SliceWeakHashTable : public RefCounted> { public: + SliceWeakHashTable() = default; + ~SliceWeakHashTable() = default; + /// Creates a new table of at most \a size entries. static RefCountedPtr Create() { return MakeRefCounted>(); @@ -61,12 +64,6 @@ class SliceWeakHashTable : public RefCounted> { } private: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - - SliceWeakHashTable() = default; - ~SliceWeakHashTable() = default; - /// The type of the table "rows". class Entry { public: diff --git a/src/core/tsi/ssl/session_cache/ssl_session_cache.h b/src/core/tsi/ssl/session_cache/ssl_session_cache.h index 16ab97714a7..0808539a07e 100644 --- a/src/core/tsi/ssl/session_cache/ssl_session_cache.h +++ b/src/core/tsi/ssl/session_cache/ssl_session_cache.h @@ -53,6 +53,9 @@ class SslSessionLRUCache : public grpc_core::RefCounted { return grpc_core::MakeRefCounted(capacity); } + explicit SslSessionLRUCache(size_t capacity); + ~SslSessionLRUCache(); + // Not copyable nor movable. SslSessionLRUCache(const SslSessionLRUCache&) = delete; SslSessionLRUCache& operator=(const SslSessionLRUCache&) = delete; @@ -67,14 +70,8 @@ class SslSessionLRUCache : public grpc_core::RefCounted { SslSessionPtr Get(const char* key); private: - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW - GRPC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE - class Node; - explicit SslSessionLRUCache(size_t capacity); - ~SslSessionLRUCache(); - Node* FindLocked(const grpc_slice& key); void Remove(Node* node); void PushFront(Node* node); From 334fa41ec092be984b011e88283c6af07d42eab9 Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Thu, 3 Oct 2019 12:14:10 -0700 Subject: [PATCH 2/2] Update by review --- src/core/lib/iomgr/buffer_list.h | 7 ++++--- src/core/lib/iomgr/cfstream_handle.h | 5 +++-- src/core/lib/slice/slice.cc | 1 - src/core/lib/slice/slice_hash_table.h | 7 ++++--- src/core/lib/slice/slice_weak_hash_table.h | 7 ++++--- src/core/tsi/ssl/session_cache/ssl_session_cache.h | 1 + 6 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/core/lib/iomgr/buffer_list.h b/src/core/lib/iomgr/buffer_list.h index 1e851dc4c9d..8818f3c3b64 100644 --- a/src/core/lib/iomgr/buffer_list.h +++ b/src/core/lib/iomgr/buffer_list.h @@ -114,6 +114,10 @@ struct Timestamps { #ifdef GRPC_LINUX_ERRQUEUE class TracedBuffer { public: + /** Use AddNewEntry function instead of using this directly. */ + TracedBuffer(uint32_t seq_no, void* arg) + : seq_no_(seq_no), arg_(arg), next_(nullptr) {} + /** Add a new entry in the TracedBuffer list pointed to by head. Also saves * sendmsg_time with the current timestamp. */ static void AddNewEntry(grpc_core::TracedBuffer** head, uint32_t seq_no, @@ -132,9 +136,6 @@ class TracedBuffer { static void Shutdown(grpc_core::TracedBuffer** head, void* remaining, grpc_error* shutdown_err); - TracedBuffer(uint32_t seq_no, void* arg) - : seq_no_(seq_no), arg_(arg), next_(nullptr) {} - private: uint32_t seq_no_; /* The sequence number for the last byte in the buffer */ void* arg_; /* The arg to pass to timestamps_callback */ diff --git a/src/core/lib/iomgr/cfstream_handle.h b/src/core/lib/iomgr/cfstream_handle.h index 96765b89037..b0f0664a04f 100644 --- a/src/core/lib/iomgr/cfstream_handle.h +++ b/src/core/lib/iomgr/cfstream_handle.h @@ -43,10 +43,12 @@ class CFStreamHandle : public GrpcLibraryInitHolder { public: static CFStreamHandle* CreateStreamHandle(CFReadStreamRef read_stream, CFWriteStreamRef write_stream); - ~CFStreamHandle() override; + /** Use CreateStreamHandle function instead of using this directly. */ + CFStreamHandle(CFReadStreamRef read_stream, CFWriteStreamRef write_stream); CFStreamHandle(const CFStreamHandle& ref) = delete; CFStreamHandle(CFStreamHandle&& ref) = delete; CFStreamHandle& operator=(const CFStreamHandle& rhs) = delete; + ~CFStreamHandle() override; void NotifyOnOpen(grpc_closure* closure); void NotifyOnRead(grpc_closure* closure); @@ -57,7 +59,6 @@ class CFStreamHandle : public GrpcLibraryInitHolder { void Unref(const char* file = "", int line = 0, const char* reason = nullptr); private: - CFStreamHandle(CFReadStreamRef read_stream, CFWriteStreamRef write_stream); static void ReadCallback(CFReadStreamRef stream, CFStreamEventType type, void* client_callback_info); static void WriteCallback(CFWriteStreamRef stream, CFStreamEventType type, diff --git a/src/core/lib/slice/slice.cc b/src/core/lib/slice/slice.cc index 5cafb4a6f3f..a3936a8d2e3 100644 --- a/src/core/lib/slice/slice.cc +++ b/src/core/lib/slice/slice.cc @@ -87,7 +87,6 @@ class NewSliceRefcount { grpc_slice_refcount* base_refcount() { return &base_; } private: - grpc_slice_refcount base_; RefCount refs_; void (*user_destroy_)(void*); diff --git a/src/core/lib/slice/slice_hash_table.h b/src/core/lib/slice/slice_hash_table.h index f94fd08cad0..29a5fc6dd24 100644 --- a/src/core/lib/slice/slice_hash_table.h +++ b/src/core/lib/slice/slice_hash_table.h @@ -65,6 +65,10 @@ class SliceHashTable : public RefCounted> { Entry* entries, ValueCmp value_cmp); + // Use Create function instead of using this directly. + SliceHashTable(size_t num_entries, Entry* entries, ValueCmp value_cmp); + virtual ~SliceHashTable(); + /// Returns the value from the table associated with \a key. /// Returns null if \a key is not found. const T* Get(const grpc_slice& key) const; @@ -77,9 +81,6 @@ class SliceHashTable : public RefCounted> { /// - else, if value_cmp(a_value, b_value) < 1 (resp. > 1). static int Cmp(const SliceHashTable& a, const SliceHashTable& b); - SliceHashTable(size_t num_entries, Entry* entries, ValueCmp value_cmp); - virtual ~SliceHashTable(); - private: void Add(const grpc_slice& key, T& value); diff --git a/src/core/lib/slice/slice_weak_hash_table.h b/src/core/lib/slice/slice_weak_hash_table.h index 0d691566033..931e405c1e3 100644 --- a/src/core/lib/slice/slice_weak_hash_table.h +++ b/src/core/lib/slice/slice_weak_hash_table.h @@ -39,14 +39,15 @@ namespace grpc_core { template class SliceWeakHashTable : public RefCounted> { public: - SliceWeakHashTable() = default; - ~SliceWeakHashTable() = default; - /// Creates a new table of at most \a size entries. static RefCountedPtr Create() { return MakeRefCounted>(); } + /// Use Create function instead of using this directly. + SliceWeakHashTable() = default; + ~SliceWeakHashTable() = default; + /// Add a mapping from \a key to \a value, taking ownership of \a key. This /// operation will always succeed. It may discard older entries. void Add(const grpc_slice& key, T value) { diff --git a/src/core/tsi/ssl/session_cache/ssl_session_cache.h b/src/core/tsi/ssl/session_cache/ssl_session_cache.h index 0808539a07e..2eadab25edd 100644 --- a/src/core/tsi/ssl/session_cache/ssl_session_cache.h +++ b/src/core/tsi/ssl/session_cache/ssl_session_cache.h @@ -53,6 +53,7 @@ class SslSessionLRUCache : public grpc_core::RefCounted { return grpc_core::MakeRefCounted(capacity); } + // Use Create function instead of using this directly. explicit SslSessionLRUCache(size_t capacity); ~SslSessionLRUCache();