From fcd1cee52838b4efe09a63a3312b627880027fcb Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Mon, 4 Sep 2023 05:06:29 -0700 Subject: [PATCH] Reserve/InternalExtend use capacity instead of size as input. This allows to remove branch from `InternalExtend` which is beneficial for small fields. Also refactored `UnsafeArenaAddAllocated` - it looks like it had an outdated comment. The function no longer deletes already allocated elements and hence does not need to be a template. PiperOrigin-RevId: 562530131 --- .../protobuf/generated_message_reflection.cc | 4 +- .../protobuf/repeated_field_unittest.cc | 4 +- src/google/protobuf/repeated_ptr_field.cc | 114 ++++------ src/google/protobuf/repeated_ptr_field.h | 199 +++++++----------- 4 files changed, 118 insertions(+), 203 deletions(-) diff --git a/src/google/protobuf/generated_message_reflection.cc b/src/google/protobuf/generated_message_reflection.cc index 5cd3830fd9cfc..9aab4eee3fc16 100644 --- a/src/google/protobuf/generated_message_reflection.cc +++ b/src/google/protobuf/generated_message_reflection.cc @@ -2484,7 +2484,7 @@ Message* Reflection::AddMessage(Message* message, const FieldDescriptor* field, // We can guarantee here that repeated and result are either both heap // allocated or arena owned. So it is safe to call the unsafe version // of AddAllocated. - repeated->UnsafeArenaAddAllocated >(result); + repeated->UnsafeArenaAddAllocated(result); } return result; @@ -2526,7 +2526,7 @@ void Reflection::UnsafeArenaAddAllocatedMessage(Message* message, } else { repeated = MutableRaw(message, field); } - repeated->UnsafeArenaAddAllocated>(new_entry); + repeated->UnsafeArenaAddAllocated(new_entry); } } diff --git a/src/google/protobuf/repeated_field_unittest.cc b/src/google/protobuf/repeated_field_unittest.cc index 52f85df5102d1..0aa3b00a1bbde 100644 --- a/src/google/protobuf/repeated_field_unittest.cc +++ b/src/google/protobuf/repeated_field_unittest.cc @@ -1694,8 +1694,8 @@ TEST(RepeatedPtrField, AddAllocated) { std::string* moo = new std::string("moo"); field.AddAllocated(moo); EXPECT_EQ(index + 1, field.size()); - // We should have discarded the cleared object. - EXPECT_EQ(0, field.ClearedCount()); + // We should have retained the cleared object. + EXPECT_EQ(1, field.ClearedCount()); EXPECT_EQ(moo, &field.Get(index)); } diff --git a/src/google/protobuf/repeated_ptr_field.cc b/src/google/protobuf/repeated_ptr_field.cc index 36dac0e0f38c7..046b0bfc0370c 100644 --- a/src/google/protobuf/repeated_ptr_field.cc +++ b/src/google/protobuf/repeated_ptr_field.cc @@ -52,46 +52,52 @@ namespace protobuf { namespace internal { -void** RepeatedPtrFieldBase::InternalExtend(int extend_amount) { - int new_size = current_size_ + extend_amount; - if (total_size_ >= new_size) { - // N.B.: rep_ is non-nullptr because extend_amount is always > 0, hence - // total_size must be non-zero since it is lower-bounded by new_size. - return elements() + current_size_; +void RepeatedPtrFieldBase::UnsafeArenaAddAllocated(void* value) { + MaybeExtend(); + void** elems = elements(); + if (current_size_ < allocated_size_) { + // We have some cleared objects. We don't care about their order, so we + // can just move the first one to the end to make space. + elems[allocated_size_] = elems[current_size_]; } + ++allocated_size_; + elems[ExchangeCurrentSize(current_size_ + 1)] = value; +} + +void RepeatedPtrFieldBase::InternalExtend(int extend_amount) { + ABSL_DCHECK(extend_amount > 0); constexpr size_t ptr_size = sizeof(rep()->elements[0]); + int new_capacity = Capacity() + extend_amount; Arena* arena = GetOwningArena(); - new_size = internal::CalculateReserveSize(total_size_, - new_size); + new_capacity = internal::CalculateReserveSize( + Capacity(), new_capacity); ABSL_CHECK_LE( - static_cast(new_size), + static_cast(new_capacity), static_cast( (std::numeric_limits::max() - kRepHeaderSize) / ptr_size)) << "Requested size is too large to fit into size_t."; - size_t bytes = kRepHeaderSize + ptr_size * new_size; + size_t bytes = kRepHeaderSize + ptr_size * new_capacity; Rep* new_rep; void* old_tagged_ptr = tagged_rep_or_elem_; if (arena == nullptr) { internal::SizedPtr res = internal::AllocateAtLeast(bytes); - new_size = static_cast((res.n - kRepHeaderSize) / ptr_size); new_rep = reinterpret_cast(res.p); + new_rep->capacity = static_cast((res.n - kRepHeaderSize) / ptr_size); } else { new_rep = reinterpret_cast(Arena::CreateArray(arena, bytes)); + new_rep->capacity = new_capacity; } if (using_sso()) { - new_rep->allocated_size = old_tagged_ptr != nullptr ? 1 : 0; new_rep->elements[0] = old_tagged_ptr; } else { Rep* old_rep = reinterpret_cast(reinterpret_cast(old_tagged_ptr) - 1); - if (old_rep->allocated_size > 0) { - memcpy(new_rep->elements, old_rep->elements, - old_rep->allocated_size * ptr_size); + if (allocated_size_ > 0) { + memcpy(new_rep->elements, old_rep->elements, allocated_size_ * ptr_size); } - new_rep->allocated_size = old_rep->allocated_size; - size_t old_size = total_size_ * ptr_size + kRepHeaderSize; + size_t old_size = Capacity() * ptr_size + kRepHeaderSize; if (arena == nullptr) { internal::SizedDelete(old_rep, old_size); } else { @@ -101,88 +107,48 @@ void** RepeatedPtrFieldBase::InternalExtend(int extend_amount) { tagged_rep_or_elem_ = reinterpret_cast(reinterpret_cast(new_rep) + 1); - total_size_ = new_size; - return &new_rep->elements[current_size_]; } -void RepeatedPtrFieldBase::Reserve(int new_size) { - if (new_size > current_size_) { - InternalExtend(new_size - current_size_); +void RepeatedPtrFieldBase::Reserve(int capacity) { + int n = Capacity(); + if (capacity > n) { + InternalExtend(capacity - n); } } void RepeatedPtrFieldBase::DestroyProtos() { - ABSL_DCHECK(tagged_rep_or_elem_); ABSL_DCHECK(arena_ == nullptr); - if (using_sso()) { - delete static_cast(tagged_rep_or_elem_); - - } else { + void** elems = elements(); + for (int i = 0; i < allocated_size_; i++) { + delete static_cast(elems[i]); + } + if (!using_sso()) { 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 = total_size_ * sizeof(elements[0]) + kRepHeaderSize; - internal::SizedDelete(r, size); - tagged_rep_or_elem_ = nullptr; + internal::SizedDelete(r, r->capacity * sizeof(elems[0]) + kRepHeaderSize); } } void* RepeatedPtrFieldBase::AddOutOfLineHelper(void* obj) { - if (tagged_rep_or_elem_ == nullptr) { - ABSL_DCHECK_EQ(current_size_, 0); - ABSL_DCHECK(using_sso()); - ABSL_DCHECK_EQ(allocated_size(), 0); - ExchangeCurrentSize(1); - tagged_rep_or_elem_ = obj; - return obj; - } - if (using_sso() || rep()->allocated_size == total_size_) { - InternalExtend(1); // Equivalent to "Reserve(total_size_ + 1)" - } - Rep* r = rep(); - ++r->allocated_size; - r->elements[ExchangeCurrentSize(current_size_ + 1)] = obj; - return obj; + return AddAllocatedForParse(obj); } void RepeatedPtrFieldBase::CloseGap(int start, int num) { - if (using_sso()) { - if (start == 0 && num == 1) { - tagged_rep_or_elem_ = nullptr; - } - } else { // Close up a gap of "num" elements starting at offset "start". - Rep* r = rep(); - for (int i = start + num; i < r->allocated_size; ++i) - r->elements[i - num] = r->elements[i]; - r->allocated_size -= num; - } + void** elems = elements(); + for (int i = start + num; i < allocated_size_; ++i) elems[i - num] = elems[i]; + allocated_size_ -= num; ExchangeCurrentSize(current_size_ - num); } MessageLite* RepeatedPtrFieldBase::AddWeak(const MessageLite* prototype) { - if (current_size_ < allocated_size()) { - return reinterpret_cast( + if (current_size_ < allocated_size_) { + return static_cast( element_at(ExchangeCurrentSize(current_size_ + 1))); } - if (allocated_size() == total_size_) { - Reserve(total_size_ + 1); - } MessageLite* result = prototype ? prototype->New(arena_) : Arena::CreateMessage(arena_); - if (using_sso()) { - ExchangeCurrentSize(current_size_ + 1); - tagged_rep_or_elem_ = result; - } else { - Rep* r = rep(); - ++r->allocated_size; - r->elements[ExchangeCurrentSize(current_size_ + 1)] = result; - } - return result; + return static_cast(AddAllocatedForParse(result)); } void InternalOutOfLineDeleteMessageLite(MessageLite* message) { diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h index 98398526c5ea5..d2d34657d1402 100644 --- a/src/google/protobuf/repeated_ptr_field.h +++ b/src/google/protobuf/repeated_ptr_field.h @@ -176,15 +176,15 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { protected: constexpr RepeatedPtrFieldBase() - : arena_(nullptr), - current_size_(0), - total_size_(kSSOCapacity), - tagged_rep_or_elem_(nullptr) {} + : current_size_(0), + allocated_size_(0), + tagged_rep_or_elem_(nullptr), + arena_(nullptr) {} explicit RepeatedPtrFieldBase(Arena* arena) - : arena_(arena), - current_size_(0), - total_size_(kSSOCapacity), - tagged_rep_or_elem_(nullptr) {} + : current_size_(0), + allocated_size_(0), + tagged_rep_or_elem_(nullptr), + arena_(arena) {} RepeatedPtrFieldBase(const RepeatedPtrFieldBase&) = delete; RepeatedPtrFieldBase& operator=(const RepeatedPtrFieldBase&) = delete; @@ -199,7 +199,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { bool empty() const { return current_size_ == 0; } int size() const { return current_size_; } - int Capacity() const { return total_size_; } + int Capacity() const { return using_sso() ? kSSOCapacity : rep()->capacity; } template const Value& at(int index) const { @@ -224,7 +224,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { template Value* Add(const Value* prototype = nullptr) { - if (current_size_ < allocated_size()) { + if (current_size_ < allocated_size_) { return cast( element_at(ExchangeCurrentSize(current_size_ + 1))); } @@ -236,15 +236,13 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { typename TypeHandler, typename std::enable_if::type* = nullptr> inline void Add(Value&& value) { - if (current_size_ < allocated_size()) { + if (current_size_ < allocated_size_) { *cast(element_at(ExchangeCurrentSize(current_size_ + 1))) = std::move(value); return; } - if (allocated_size() == total_size_) { - Reserve(total_size_ + 1); - } - if (!using_sso()) ++rep()->allocated_size; + MaybeExtend(); + ++allocated_size_; auto* result = TypeHandler::New(arena_, std::move(value)); element_at(ExchangeCurrentSize(current_size_ + 1)) = result; } @@ -261,24 +259,17 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { void Destroy() { if (arena_ != nullptr) return; - if (using_sso()) { - if (tagged_rep_or_elem_ == nullptr) return; - Delete(tagged_rep_or_elem_, nullptr); - return; - } - - Rep* r = rep(); - int n = r->allocated_size; - void* const* elems = r->elements; - for (int i = 0; i < n; i++) { + void** elems = elements(); + for (int i = 0; i < allocated_size_; i++) { Delete(elems[i], nullptr); } - internal::SizedDelete(r, total_size_ * sizeof(elems[0]) + kRepHeaderSize); + if (!using_sso()) { + Rep* r = rep(); + internal::SizedDelete(r, r->capacity * sizeof(elems[0]) + kRepHeaderSize); + } } - bool NeedsDestroy() const { - return tagged_rep_or_elem_ != nullptr && arena_ == nullptr; - } + bool NeedsDestroy() const { return arena_ == nullptr; } void DestroyProtos(); // implemented in the cc file public: @@ -334,20 +325,16 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { // It ensures we have no preallocated elements in the array. // Returns true if the invariants hold and `AddAllocatedForParse` can be // used. - bool PrepareForParse() { return allocated_size() == current_size_; } + bool PrepareForParse() { return allocated_size_ == current_size_; } // Similar to `AddAllocated` but faster. // Can only be invoked after a call to `PrepareForParse` that returned `true`, // or other calls to `AddAllocatedForParse`. - template - void AddAllocatedForParse(Value* value) { - ABSL_DCHECK_EQ(current_size_, allocated_size()); - if (current_size_ == total_size_) { - // The array is completely full with no cleared objects, so grow it. - InternalExtend(1); - } - element_at(current_size_++) = value; - if (!using_sso()) ++rep()->allocated_size; + inline void* AddAllocatedForParse(void* value) { + ABSL_DCHECK_EQ(current_size_, allocated_size_); + MaybeExtend(); + ++allocated_size_; + return element_at(current_size_++) = value; } protected: @@ -367,7 +354,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { void CloseGap(int start, int num); // implemented in the cc file - void Reserve(int new_size); // implemented in the cc file + void Reserve(int capacity); // implemented in the cc file template static inline Value* copy(Value* value) { @@ -419,10 +406,9 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { size_t allocated_bytes = using_sso() ? 0 - : static_cast(total_size_) * sizeof(void*) + kRepHeaderSize; - const int n = allocated_size(); + : static_cast(Capacity()) * sizeof(void*) + kRepHeaderSize; void* const* elems = elements(); - for (int i = 0; i < n; ++i) { + for (int i = 0; i < allocated_size_; ++i) { allocated_bytes += TypeHandler::SpaceUsedLong(*cast(elems[i])); } @@ -434,7 +420,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { // Like Add(), but if there are no cleared objects to use, returns nullptr. template Value* AddFromCleared() { - if (current_size_ < allocated_size()) { + if (current_size_ < allocated_size_) { return cast( element_at(ExchangeCurrentSize(current_size_ + 1))); } else { @@ -448,31 +434,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { AddAllocatedInternal(value, t); } - template - void UnsafeArenaAddAllocated(Value* value) { - // Make room for the new pointer. - if (current_size_ == total_size_) { - // The array is completely full with no cleared objects, so grow it. - Reserve(total_size_ + 1); - ++rep()->allocated_size; - } else if (allocated_size() == total_size_) { - // There is no more space in the pointer array because it contains some - // cleared objects awaiting reuse. We don't want to grow the array in - // this case because otherwise a loop calling AddAllocated() followed by - // Clear() would leak memory. - Delete(element_at(current_size_), arena_); - } else if (current_size_ < allocated_size()) { - // We have some cleared objects. We don't care about their order, so we - // can just move the first one to the end to make space. - element_at(allocated_size()) = element_at(current_size_); - ++rep()->allocated_size; - } else { - // There are no cleared objects. - if (!using_sso()) ++rep()->allocated_size; - } - - element_at(ExchangeCurrentSize(current_size_ + 1)) = value; - } + void UnsafeArenaAddAllocated(void* value); template PROTOBUF_NODISCARD Value* ReleaseLast() { @@ -488,20 +450,15 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { ABSL_DCHECK_GT(current_size_, 0); ExchangeCurrentSize(current_size_ - 1); auto* result = cast(element_at(current_size_)); - if (using_sso()) { - tagged_rep_or_elem_ = nullptr; - } else { - --rep()->allocated_size; - if (current_size_ < allocated_size()) { - // There are cleared elements on the end; replace the removed element - // with the last allocated element. - element_at(current_size_) = element_at(allocated_size()); - } + if (current_size_ < --allocated_size_) { + // There are cleared elements on the end; replace the removed element + // with the last allocated element. + element_at(current_size_) = element_at(allocated_size_); } return result; } - int ClearedCount() const { return allocated_size() - current_size_; } + int ClearedCount() const { return allocated_size_ - current_size_; } template void AddCleared(Value* value) { @@ -510,14 +467,8 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { "RepeatedPtrField not on an arena."; ABSL_DCHECK(TypeHandler::GetOwningArena(value) == nullptr) << "AddCleared() can only accept values not on an arena."; - if (allocated_size() == total_size_) { - Reserve(total_size_ + 1); - } - if (using_sso()) { - tagged_rep_or_elem_ = value; - } else { - element_at(rep()->allocated_size++) = value; - } + MaybeExtend(); + element_at(allocated_size_++) = value; } template @@ -526,14 +477,8 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { << "ReleaseCleared() can only be used on a RepeatedPtrField not on " << "an arena."; ABSL_DCHECK(tagged_rep_or_elem_ != nullptr); - ABSL_DCHECK_GT(allocated_size(), current_size_); - if (using_sso()) { - auto* result = cast(tagged_rep_or_elem_); - tagged_rep_or_elem_ = nullptr; - return result; - } else { - return cast(element_at(--rep()->allocated_size)); - } + ABSL_DCHECK_GT(allocated_size_, current_size_); + return cast(element_at(--allocated_size_)); } // AddAllocated version that implements arena-safe copying behavior. @@ -541,18 +486,18 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { void AddAllocatedInternal(Value* value, std::true_type) { Arena* element_arena = TypeHandler::GetOwningArena(value); Arena* arena = GetOwningArena(); - if (arena == element_arena && allocated_size() < total_size_) { + if (arena == element_arena && allocated_size_ < Capacity()) { // Fast path: underlying arena representation (tagged pointer) is equal to // our arena pointer, and we can add to array without resizing it (at // least one slot that is not allocated). void** elems = elements(); - if (current_size_ < allocated_size()) { + if (current_size_ < allocated_size_) { // Make space at [current] by moving first allocated element to end of // allocated list. - elems[allocated_size()] = elems[current_size_]; + elems[allocated_size_] = elems[current_size_]; } elems[ExchangeCurrentSize(current_size_ + 1)] = value; - if (!using_sso()) ++rep()->allocated_size; + ++allocated_size_; } else { AddAllocatedSlowWithCopy(value, element_arena, arena); } @@ -561,20 +506,20 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { // AddAllocated version that does not implement arena-safe copying behavior. template void AddAllocatedInternal(Value* value, std::false_type) { - if (allocated_size() < total_size_) { + if (allocated_size_ < Capacity()) { // Fast path: underlying arena representation (tagged pointer) is equal to // our arena pointer, and we can add to array without resizing it (at // least one slot that is not allocated). void** elems = elements(); - if (current_size_ < allocated_size()) { + if (current_size_ < allocated_size_) { // Make space at [current] by moving first allocated element to end of // allocated list. - elems[allocated_size()] = elems[current_size_]; + elems[allocated_size_] = elems[current_size_]; } elems[ExchangeCurrentSize(current_size_ + 1)] = value; - if (!using_sso()) ++rep()->allocated_size; + ++allocated_size_; } else { - UnsafeArenaAddAllocated(value); + UnsafeArenaAddAllocated(value); } } @@ -596,7 +541,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { value = new_value; } - UnsafeArenaAddAllocated(value); + UnsafeArenaAddAllocated(value); } template @@ -692,7 +637,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { friend class internal::SwapFieldHelper; struct Rep { - int allocated_size; + int capacity; // Here we declare a huge array as a way of approximating C's "flexible // array member" feature without relying on undefined behavior. void* elements[(std::numeric_limits::max() - 2 * sizeof(int)) / @@ -727,10 +672,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { return const_cast(this)->element_at(index); } - int allocated_size() const { - return using_sso() ? (tagged_rep_or_elem_ != nullptr ? 1 : 0) - : rep()->allocated_size; - } Rep* rep() { ABSL_DCHECK(!using_sso()); return reinterpret_cast( @@ -779,16 +720,17 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { const RepeatedPtrFieldBase& other, void (RepeatedPtrFieldBase::*inner_loop)(void**, void* const*, int, int)) { - // Note: wrapper has already guaranteed that other.rep_ != nullptr here. + // 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 = InternalExtend(other_size); - int allocated_elems = allocated_size() - current_size_; + 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_; + if (allocated_size_ < current_size_) { + allocated_size_ = current_size_; } } @@ -814,12 +756,19 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { } } - // Internal helper: extends array space if necessary to contain - // |extend_amount| more elements, and returns a pointer to the element - // immediately following the old list of elements. This interface factors out - // common behavior from Reserve() and MergeFrom() to reduce code size. - // |extend_amount| must be > 0. - void** InternalExtend(int extend_amount); + // Extends capacity by at least |extend_amount|. + // + // Pre-condition: |extend_amount| must be > 0. + void InternalExtend(int extend_amount); + + // Ensures that capacity is big enough to store one more allocated element. + PROTOBUF_ALWAYS_INLINE inline void MaybeExtend() { + // Capacity grows exponentially, hence extending is not needed most of the + // time. + if (PROTOBUF_PREDICT_FALSE(allocated_size_ == Capacity())) { + InternalExtend(1); + } + } // Internal helper for Add: adds "obj" as the next element in the // array, including potentially resizing the array with Reserve if @@ -837,10 +786,10 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { // misses due to the indirection, because these fields are checked frequently. // Placing all fields directly in the RepeatedPtrFieldBase instance would cost // significant performance for memory-sensitive workloads. - Arena* arena_; int current_size_; - int total_size_; + int allocated_size_; void* tagged_rep_or_elem_; + Arena* arena_; }; void InternalOutOfLineDeleteMessageLite(MessageLite* message); @@ -1309,7 +1258,7 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase { std::false_type); void AddAllocatedForParse(Element* p) { - return RepeatedPtrFieldBase::AddAllocatedForParse(p); + RepeatedPtrFieldBase::AddAllocatedForParse(p); } }; @@ -1675,7 +1624,7 @@ inline void RepeatedPtrField::AddAllocated(Element* value) { template inline void RepeatedPtrField::UnsafeArenaAddAllocated(Element* value) { - RepeatedPtrFieldBase::UnsafeArenaAddAllocated(value); + RepeatedPtrFieldBase::UnsafeArenaAddAllocated(value); } template