From 17e06c108dc8a3f43ea0f909999d74d7166f9733 Mon Sep 17 00:00:00 2001 From: Martijn Vels Date: Wed, 4 Oct 2023 09:46:33 -0700 Subject: [PATCH] Rollback RepeatedPtrField optimization PiperOrigin-RevId: 570722595 --- src/google/protobuf/implicit_weak_message.h | 3 +- src/google/protobuf/repeated_ptr_field.cc | 91 +------------- src/google/protobuf/repeated_ptr_field.h | 127 +++++++++----------- 3 files changed, 61 insertions(+), 160 deletions(-) diff --git a/src/google/protobuf/implicit_weak_message.h b/src/google/protobuf/implicit_weak_message.h index 170155a2c9..9b782ed95c 100644 --- a/src/google/protobuf/implicit_weak_message.h +++ b/src/google/protobuf/implicit_weak_message.h @@ -186,8 +186,7 @@ struct WeakRepeatedPtrField { T* Add() { return weak.Add(); } void Clear() { base().template Clear(); } void MergeFrom(const WeakRepeatedPtrField& other) { - if (other.empty()) return; - base().template MergeFrom(other.base()); + base().template MergeFrom(other.base()); } void InternalSwap(WeakRepeatedPtrField* PROTOBUF_RESTRICT other) { base().InternalSwap(&other->base()); diff --git a/src/google/protobuf/repeated_ptr_field.cc b/src/google/protobuf/repeated_ptr_field.cc index f444ef4e91..e9d70f0b59 100644 --- a/src/google/protobuf/repeated_ptr_field.cc +++ b/src/google/protobuf/repeated_ptr_field.cc @@ -9,12 +9,10 @@ // Based on original Protocol Buffers design by // Sanjay Ghemawat, Jeff Dean, and others. -#include #include #include #include #include -#include #include "absl/log/absl_check.h" #include "google/protobuf/arena.h" @@ -30,7 +28,7 @@ namespace protobuf { namespace internal { -void** RepeatedPtrFieldBase::InternalExtend(int extend_amount) { +void RepeatedPtrFieldBase::InternalExtend(int extend_amount) { ABSL_DCHECK(extend_amount > 0); constexpr size_t ptr_size = sizeof(rep()->elements[0]); int new_capacity = total_size_ + extend_amount; @@ -76,7 +74,6 @@ void** RepeatedPtrFieldBase::InternalExtend(int extend_amount) { tagged_rep_or_elem_ = reinterpret_cast(reinterpret_cast(new_rep) + 1); total_size_ = new_capacity; - return &new_rep->elements[current_size_]; } void RepeatedPtrFieldBase::Reserve(int capacity) { @@ -154,92 +151,8 @@ template PROTOBUF_EXPORT_TEMPLATE_DEFINE void memswap::value>( char* PROTOBUF_RESTRICT, char* PROTOBUF_RESTRICT); -template <> -void RepeatedPtrFieldBase::MergeFrom( - const RepeatedPtrFieldBase& from) { - ABSL_DCHECK_NE(&from, this); - int new_size = current_size_ + from.current_size_; - auto dst = reinterpret_cast(InternalReserve(new_size)); - auto src = reinterpret_cast(from.elements()); - auto end = src + from.current_size_; - auto end_assign = src + std::min(ClearedCount(), from.current_size_); - for (; src < end_assign; ++dst, ++src) { - (*dst)->assign(**src); - } - if (Arena* const arena = arena_) { - for (; src < end; ++dst, ++src) { - *dst = Arena::Create(arena, **src); - } - } else { - for (; src < end; ++dst, ++src) { - *dst = new std::string(**src); - } - } - ExchangeCurrentSize(new_size); - if (new_size > allocated_size()) { - rep()->allocated_size = new_size; - } -} - - -int RepeatedPtrFieldBase::MergeIntoClearedMessages( - const RepeatedPtrFieldBase& from) { - auto dst = reinterpret_cast(elements() + current_size_); - auto src = reinterpret_cast(from.elements()); - int count = std::min(ClearedCount(), from.current_size_); - for (int i = 0; i < count; ++i) { - dst[i]->CheckTypeAndMergeFrom(*src[i]); - } - return count; -} - -void RepeatedPtrFieldBase::MergeFromConcreteMessage( - const RepeatedPtrFieldBase& from, CopyFn copy_fn) { - ABSL_DCHECK_NE(&from, this); - int new_size = current_size_ + from.current_size_; - auto dst = reinterpret_cast(InternalReserve(new_size)); - auto src = reinterpret_cast(from.elements()); - auto end = src + from.current_size_; - if (PROTOBUF_PREDICT_FALSE(ClearedCount() > 0)) { - int recycled = MergeIntoClearedMessages(from); - dst += recycled; - src += recycled; - } - Arena* arena = GetOwningArena(); - for (; src < end; ++src, ++dst) { - *dst = copy_fn(arena, **src); - } - ExchangeCurrentSize(new_size); - if (new_size > allocated_size()) { - rep()->allocated_size = new_size; - } -} - -template <> -void RepeatedPtrFieldBase::MergeFrom( - const RepeatedPtrFieldBase& from) { - ABSL_DCHECK_NE(&from, this); - int new_size = current_size_ + from.current_size_; - auto dst = reinterpret_cast(InternalReserve(new_size)); - auto src = reinterpret_cast(from.elements()); - auto end = src + from.current_size_; - if (PROTOBUF_PREDICT_FALSE(ClearedCount() > 0)) { - int recycled = MergeIntoClearedMessages(from); - dst += recycled; - src += recycled; - } - Arena* arena = GetOwningArena(); - for (; src < end; ++src, ++dst) { - *dst = (*src)->New(arena); - (*dst)->CheckTypeAndMergeFrom(**src); - } - ExchangeCurrentSize(new_size); - if (new_size > allocated_size()) { - rep()->allocated_size = new_size; - } -} - } // namespace internal + } // namespace protobuf } // namespace google diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h index 51c8065685..7c1e458149 100644 --- a/src/google/protobuf/repeated_ptr_field.h +++ b/src/google/protobuf/repeated_ptr_field.h @@ -296,19 +296,18 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { } } - // Message creating functor: used in MergeFrom() - template - static MessageLite* CopyMessage(Arena* arena, const MessageLite& src) { - T* msg = Arena::CreateMaybeMessage(arena); - msg->MergeFrom(static_cast(src)); - return msg; - } - - // Appends all message values from `from` to this instance. - template - void MergeFrom(const RepeatedPtrFieldBase& from) { - static_assert(std::is_base_of::value, ""); - MergeFromConcreteMessage(from, CopyMessage); + template + void MergeFrom(const RepeatedPtrFieldBase& other) { + // To avoid unnecessary code duplication and reduce binary size, we use a + // layered approach to implementing MergeFrom(). The toplevel method is + // templated, so we get a small thunk per concrete message type in the + // binary. This calls a shared implementation with most of the logic, + // passing a function pointer to another type-specific piece of code that + // calls the object-allocate and merge handlers. + ABSL_DCHECK_NE(&other, this); + if (other.current_size_ == 0) return; + using H = CommonHandler; + MergeFromInternal(other, &RepeatedPtrFieldBase::MergeFromInnerLoop); } inline void InternalSwap(RepeatedPtrFieldBase* PROTOBUF_RESTRICT rhs) { @@ -349,8 +348,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { void CopyFrom(const RepeatedPtrFieldBase& other) { if (&other == this) return; RepeatedPtrFieldBase::Clear(); - if (other.empty()) return; - RepeatedPtrFieldBase::MergeFrom(other); + RepeatedPtrFieldBase::MergeFrom(other); } void CloseGap(int start, int num); @@ -631,10 +629,9 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { // temporary on |other|'s arena so that messages are copied twice rather // than three times. RepeatedPtrFieldBase temp(other->GetOwningArena()); - if (!this->empty()) { - temp.MergeFrom(*this); - } - this->CopyFrom(*other); + temp.MergeFrom(*this); + this->Clear(); + this->MergeFrom(*other); other->InternalSwap(&temp); temp.Destroy(); // Frees rep_ if `other` had no arena. } @@ -686,12 +683,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { friend class google::protobuf::Reflection; friend class internal::SwapFieldHelper; - // Concrete Arena enabled copy function used to copy messages instances. - // This follows the `Arena::CreateMaybeMessage` signature so that the compiler - // can have the inlined call into the out of line copy function(s) simply pass - // the address of `Arena::CreateMaybeMessage` 'as is'. - using CopyFn = MessageLite* (*)(Arena*, const MessageLite&); - struct Rep { int allocated_size; // Here we declare a huge array as a way of approximating C's "flexible @@ -772,24 +763,52 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { ExchangeCurrentSize(0); } - // Merges messages from `from` into available, cleared messages sitting in the - // range `[size(), allocated_size())`. Returns the number of message merged - // which is `ClearedCount(), from.size())`. - // Note that this function does explicitly NOT update `current_size_`. - // This function is out of line as it should be the slow path: this scenario - // only happens when a caller constructs and fills a repeated field, then - // shrinks it, and then merges additional messages into it. - int MergeIntoClearedMessages(const RepeatedPtrFieldBase& from); + // Non-templated inner function to avoid code duplication. Takes a function + // pointer to the type-specific (templated) inner allocate/merge loop. + PROTOBUF_NOINLINE void MergeFromInternal( + const RepeatedPtrFieldBase& other, + void (RepeatedPtrFieldBase::*inner_loop)(void**, void* const*, int, + int)) { + // Note: wrapper has already guaranteed that `other_size` > 0. + int other_size = other.current_size_; + Reserve(current_size_ + other_size); + void* const* other_elements = other.elements(); + void** new_elements = elements() + current_size_; + int allocated_elems = allocated_size() - current_size_; + (this->*inner_loop)(new_elements, other_elements, other_size, + allocated_elems); + ExchangeCurrentSize(current_size_ + other_size); + if (allocated_size() < current_size_) { + rep()->allocated_size = current_size_; + } + } - // Appends all messages from `from` to this instance, using the - // provided `copy_fn` copy function to copy existing messages. - void MergeFromConcreteMessage(const RepeatedPtrFieldBase& from, - CopyFn copy_fn); + // Merges other_elems to our_elems. + template + PROTOBUF_NOINLINE void MergeFromInnerLoop(void** our_elems, + void* const* other_elems, + int length, int already_allocated) { + if (already_allocated < length) { + Arena* arena = GetOwningArena(); + auto* elem_prototype = cast(other_elems[0]); + for (int i = already_allocated; i < length; i++) { + // Allocate a new empty element that we'll merge into below + our_elems[i] = TypeHandler::NewFromPrototype(elem_prototype, arena); + } + } + // Main loop that does the actual merging + for (int i = 0; i < length; i++) { + // Already allocated: use existing element. + auto* other_elem = cast(other_elems[i]); + auto* new_elem = cast(our_elems[i]); + TypeHandler::Merge(*other_elem, new_elem); + } + } // Extends capacity by at least |extend_amount|. // // Pre-condition: |extend_amount| must be > 0. - void** InternalExtend(int extend_amount); + void InternalExtend(int extend_amount); // Ensures that capacity is big enough to store one more allocated element. inline void MaybeExtend() { @@ -802,16 +821,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { } } - // Ensures that capacity is at least `n` elements. - // Returns a pointer to the element directly beyond the last element. - inline void** InternalReserve(int n) { - if (n <= total_size_) { - void** elements = using_sso() ? &tagged_rep_or_elem_ : rep()->elements; - return elements + current_size_; - } - return InternalExtend(n - total_size_); - } - // Internal helper for Add: adds "obj" as the next element in the // array, including potentially resizing the array with Reserve if // needed @@ -834,25 +843,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { Arena* arena_; }; -// Appends all message values from `from` to this instance using the abstract -// message interface. This overload is used in places like reflection and -// other locations where the underlying type is unavailable -template <> -void RepeatedPtrFieldBase::MergeFrom( - const RepeatedPtrFieldBase& from); - -template <> -inline void RepeatedPtrFieldBase::MergeFrom( - const RepeatedPtrFieldBase& from) { - return MergeFrom(from); -} - -// Appends all `std::string` values from `from` to this instance. -template <> -void RepeatedPtrFieldBase::MergeFrom( - const RepeatedPtrFieldBase& from); - - void InternalOutOfLineDeleteMessageLite(MessageLite* message); template @@ -1589,8 +1579,7 @@ inline void RepeatedPtrField::Clear() { template inline void RepeatedPtrField::MergeFrom( const RepeatedPtrField& other) { - if (other.empty()) return; - RepeatedPtrFieldBase::MergeFrom(other); + RepeatedPtrFieldBase::MergeFrom(other); } template