From 70876bd87ea9050b46c8efdf191c246cf452349e Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 15 Nov 2023 07:25:54 -0800 Subject: [PATCH] Short-circuit destruction of an empty field using SSO. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` name old cpu/op new cpu/op delta BM_RepeatedPtrField_Ctor 133ns ± 2% 78ns ± 1% -41.64% (p=0.000 n=80+87) ``` PiperOrigin-RevId: 582667999 --- src/google/protobuf/implicit_weak_message.h | 6 +++++- src/google/protobuf/repeated_ptr_field.cc | 15 +-------------- src/google/protobuf/repeated_ptr_field.h | 16 ++++++++++++---- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/google/protobuf/implicit_weak_message.h b/src/google/protobuf/implicit_weak_message.h index 7c94d394f3..ebd78f1a56 100644 --- a/src/google/protobuf/implicit_weak_message.h +++ b/src/google/protobuf/implicit_weak_message.h @@ -167,7 +167,11 @@ struct WeakRepeatedPtrField { // TODO: make this constructor private explicit WeakRepeatedPtrField(Arena* arena) : weak(arena) {} - ~WeakRepeatedPtrField() { weak.template Destroy(); } + ~WeakRepeatedPtrField() { + if (weak.NeedsDestroy()) { + weak.DestroyProtos(); + } + } typedef internal::RepeatedPtrIterator iterator; typedef internal::RepeatedPtrIterator const_iterator; diff --git a/src/google/protobuf/repeated_ptr_field.cc b/src/google/protobuf/repeated_ptr_field.cc index 2a98da24d4..2afe3adf7d 100644 --- a/src/google/protobuf/repeated_ptr_field.cc +++ b/src/google/protobuf/repeated_ptr_field.cc @@ -88,20 +88,7 @@ void RepeatedPtrFieldBase::Reserve(int capacity) { } void RepeatedPtrFieldBase::DestroyProtos() { - ABSL_DCHECK(tagged_rep_or_elem_); - ABSL_DCHECK(arena_ == nullptr); - if (using_sso()) { - delete static_cast(tagged_rep_or_elem_); - } else { - Rep* r = rep(); - int n = r->allocated_size; - void* const* elements = r->elements; - for (int i = 0; i < n; i++) { - delete static_cast(elements[i]); - } - const size_t size = Capacity() * sizeof(elements[0]) + kRepHeaderSize; - internal::SizedDelete(r, size); - } + Destroy>(); // TODO: Eliminate this store when invoked from the destructor, // since it is dead. diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h index 11ddd154ec..1666c9c94e 100644 --- a/src/google/protobuf/repeated_ptr_field.h +++ b/src/google/protobuf/repeated_ptr_field.h @@ -251,10 +251,12 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { } // Must be called from destructor. + // + // Pre-condition: NeedsDestroy() returns true. template void Destroy() { + ABSL_DCHECK(NeedsDestroy()); using H = CommonHandler; - if (arena_ != nullptr) return; int n = allocated_size(); void** elems = elements(); for (int i = 0; i < n; i++) { @@ -267,7 +269,10 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { } bool NeedsDestroy() const { - return tagged_rep_or_elem_ != nullptr && arena_ == nullptr; + // TODO: arena check is redundant once all `RepeatedPtrField`s + // with non-null arena are owned by the arena. + return tagged_rep_or_elem_ != nullptr && + PROTOBUF_PREDICT_FALSE(arena_ == nullptr); } void DestroyProtos(); // implemented in the cc file @@ -630,7 +635,9 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { } this->CopyFrom(*other); other->InternalSwap(&temp); - temp.Destroy(); // Frees rep_ if `other` had no arena. + if (temp.NeedsDestroy()) { + temp.Destroy(); + } } // Gets the Arena on which this RepeatedPtrField stores its elements. @@ -1374,12 +1381,13 @@ inline RepeatedPtrField::RepeatedPtrField(Iter begin, Iter end) { template RepeatedPtrField::~RepeatedPtrField() { StaticValidityCheck(); + if (!NeedsDestroy()) return; #ifdef __cpp_if_constexpr if constexpr (std::is_base_of::value) { #else if (std::is_base_of::value) { #endif - if (NeedsDestroy()) DestroyProtos(); + DestroyProtos(); } else { Destroy(); }