From 1492fa9598a79461a79f371c64a22888a6ed6fa0 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Mon, 5 Aug 2024 16:42:05 -0700 Subject: [PATCH] Automated rollback of commit e2525e6b365459f9183cb21815f74b4bba205481. PiperOrigin-RevId: 659723808 --- src/google/protobuf/compiler/cpp/unittest.inc | 17 +- src/google/protobuf/extension_set_unittest.cc | 11 +- src/google/protobuf/repeated_field.h | 565 ++++++------------ .../protobuf/repeated_field_unittest.cc | 53 +- 4 files changed, 221 insertions(+), 425 deletions(-) diff --git a/src/google/protobuf/compiler/cpp/unittest.inc b/src/google/protobuf/compiler/cpp/unittest.inc index a417bbfb09..a6552e1e1d 100644 --- a/src/google/protobuf/compiler/cpp/unittest.inc +++ b/src/google/protobuf/compiler/cpp/unittest.inc @@ -27,9 +27,8 @@ #include #include -#include "absl/base/attributes.h" -#include "absl/strings/cord.h" #include "google/protobuf/compiler/cpp/unittest.h" +#include "absl/strings/cord.h" #include "absl/strings/string_view.h" #ifndef _MSC_VER // We exclude this large proto because it's too large for @@ -483,11 +482,25 @@ TEST(GENERATED_MESSAGE_TEST_NAME, ADLSwap) { UNITTEST::TestAllTypes message1, message2; TestUtil::SetAllFields(&message1); + // Note the address of one of the repeated fields, to verify it was swapped + // rather than copied. + const int32_t* addr = &message1.repeated_int32().Get(0); +#ifdef PROTOBUF_FORCE_COPY_IN_SWAP + const int32_t value = *addr; +#endif + using std::swap; swap(message1, message2); TestUtil::ExpectAllFieldsSet(message2); TestUtil::ExpectClear(message1); + +#ifdef PROTOBUF_FORCE_COPY_IN_SWAP + EXPECT_NE(addr, &message2.repeated_int32().Get(0)); + EXPECT_EQ(value, message2.repeated_int32().Get(0)); +#else + EXPECT_EQ(addr, &message2.repeated_int32().Get(0)); +#endif } TEST(GENERATED_MESSAGE_TEST_NAME, CopyConstructor) { diff --git a/src/google/protobuf/extension_set_unittest.cc b/src/google/protobuf/extension_set_unittest.cc index 9c06ceb4db..114f516d87 100644 --- a/src/google/protobuf/extension_set_unittest.cc +++ b/src/google/protobuf/extension_set_unittest.cc @@ -11,7 +11,6 @@ #include "google/protobuf/extension_set.h" -#include #include #include @@ -842,12 +841,10 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { const size_t old_capacity = \ message->GetRepeatedExtension(unittest::repeated_##type##_extension) \ .Capacity(); \ - if (sizeof(cpptype) > 1) { \ - EXPECT_GE( \ - old_capacity, \ - (RepeatedFieldLowerClampLimit())); \ - } \ + EXPECT_GE( \ + old_capacity, \ + (RepeatedFieldLowerClampLimit())); \ for (int i = 0; i < 16; ++i) { \ message->AddExtension(unittest::repeated_##type##_extension, value); \ } \ diff --git a/src/google/protobuf/repeated_field.h b/src/google/protobuf/repeated_field.h index 17e1baf5a6..5ff41a4c15 100644 --- a/src/google/protobuf/repeated_field.h +++ b/src/google/protobuf/repeated_field.h @@ -23,8 +23,6 @@ #include #include -#include -#include #include #include #include @@ -115,120 +113,6 @@ struct HeapRep { }; }; -// We use small object optimization (SOO) to store elements inline when possible -// for small repeated fields. We do so in order to avoid memory indirections. - -// SOO data is stored in the same space as the size/capacity ints. -enum { kSooCapacityBytes = 2 * sizeof(int) }; - -// Arena/elements pointers are aligned to at least kSooPtrAlignment bytes so we -// can use the lower bits to encode whether we're in SOO mode and if so, the -// SOO size. NOTE: we also tried using all kSooPtrMask bits to encode SOO size -// and use all ones as a sentinel value for non-SOO mode, but that was slower in -// benchmarks/loadtests. -enum { kSooPtrAlignment = sizeof(void*) }; -// The mask for the size bits in SOO mode, and also a sentinel value indicating -// that the field is not in SOO mode. -enum { kSooPtrMask = ~(kSooPtrAlignment - 1) }; -// This bit is 0 when in SOO mode and 1 when in non-SOO mode. -enum { kNotSooBit = kSooPtrAlignment >> 1 }; -// These bits are used to encode the size when in SOO mode (sizes are 0-3). -enum { kSooSizeMask = kNotSooBit - 1 }; - -// The number of elements that can be stored in the SOO rep. When -// kSooPtrAlignment == 8, this is 1 for int64_t, 2 for int32_t, 3 for bool, and -// 0 for absl::Cord. -constexpr int SooCapacityElements(size_t element_size) { - return std::min(kSooCapacityBytes / element_size, kSooSizeMask); -} - -struct LongSooRep { - // Returns char* rather than void* so callers can do pointer arithmetic. - char* elements() const { - auto ret = reinterpret_cast(elements_int & kSooPtrMask); - ABSL_DCHECK_NE(ret, nullptr); - return ret; - } - - uintptr_t elements_int; - int size; - int capacity; -}; -struct ShortSooRep { - constexpr ShortSooRep() = default; - explicit ShortSooRep(Arena* arena) - : arena_and_size(reinterpret_cast(arena)) { - ABSL_DCHECK_EQ(size(), 0); - } - - int size() const { return arena_and_size & kSooSizeMask; } - bool is_soo() const { return (arena_and_size & kNotSooBit) == 0; } - - uintptr_t arena_and_size = 0; - union { - char data[kSooCapacityBytes]; - // NOTE: in some language versions, we can't have a constexpr constructor - // if we don't initialize all fields, but `data` doesn't need to be - // initialized so initialize an empty dummy variable instead. - std::true_type dummy = {}; - }; -}; -struct SooRep { - constexpr SooRep() : short_rep() {} - explicit SooRep(Arena* arena) : short_rep(arena) {} - - bool is_soo() const { - static_assert(sizeof(LongSooRep) == sizeof(ShortSooRep), ""); - static_assert(offsetof(SooRep, long_rep) == offsetof(SooRep, short_rep), - ""); - static_assert(offsetof(LongSooRep, elements_int) == - offsetof(ShortSooRep, arena_and_size), - ""); - return short_rep.is_soo(); - } - Arena* soo_arena() const { - ABSL_DCHECK(is_soo()); - return reinterpret_cast(short_rep.arena_and_size & kSooPtrMask); - } - int size(bool is_soo) const { - ABSL_DCHECK_EQ(is_soo, this->is_soo()); -#if !defined(__clang__) && defined(__GNUC__) -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" -#endif - return is_soo ? short_rep.size() : long_rep.size; -#if !defined(__clang__) && defined(__GNUC__) -#pragma GCC diagnostic pop -#endif - } - void set_size(bool is_soo, int size) { - ABSL_DCHECK_EQ(is_soo, this->is_soo()); - if (is_soo) { - ABSL_DCHECK_LE(size, kSooSizeMask); - short_rep.arena_and_size &= kSooPtrMask; - short_rep.arena_and_size |= size; - } else { - long_rep.size = size; - } - } - // Initializes the SooRep in non-SOO mode with the given capacity and heap - // allocation. - void set_non_soo(bool was_soo, int capacity, void* elements) { - ABSL_DCHECK_EQ(was_soo, is_soo()); - ABSL_DCHECK_NE(elements, nullptr); - ABSL_DCHECK_EQ(reinterpret_cast(elements) % kSooPtrAlignment, - uintptr_t{0}); - if (was_soo) long_rep.size = short_rep.size(); - long_rep.capacity = capacity; - long_rep.elements_int = reinterpret_cast(elements) | kNotSooBit; - } - - union { - LongSooRep long_rep; - ShortSooRep short_rep; - }; -}; - } // namespace internal // RepeatedField is used to represent repeated fields of a primitive type (in @@ -434,7 +318,10 @@ class RepeatedField final // Gets the Arena on which this RepeatedField stores its elements. // Note: this can be inaccurate for split default fields so we make this // function non-const. - inline Arena* GetArena() { return GetArena(is_soo()); } + inline Arena* GetArena() { + return Capacity() == 0 ? static_cast(arena_or_elements_) + : heap_rep()->arena; + } // For internal use only. // @@ -452,35 +339,15 @@ class RepeatedField final friend class Arena; - static constexpr int kSooCapacityElements = - internal::SooCapacityElements(sizeof(Element)); - static constexpr int kInitialSize = 0; static PROTOBUF_CONSTEXPR const size_t kHeapRepHeaderSize = sizeof(HeapRep); RepeatedField(Arena* arena, const RepeatedField& rhs); RepeatedField(Arena* arena, RepeatedField&& rhs); - inline Arena* GetArena(bool is_soo) const { - return is_soo ? soo_rep_.soo_arena() : heap_rep()->arena; - } - bool is_soo() const { return soo_rep_.is_soo(); } - int size(bool is_soo) const { return soo_rep_.size(is_soo); } - int Capacity(bool is_soo) const { -#if !defined(__clang__) && defined(__GNUC__) -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" -#endif - return is_soo ? kSooCapacityElements : soo_rep_.long_rep.capacity; -#if !defined(__clang__) && defined(__GNUC__) -#pragma GCC diagnostic pop -#endif - } - void set_size(bool is_soo, int size) { - ABSL_DCHECK_LE(size, Capacity(is_soo)); - soo_rep_.set_size(is_soo, size); - } + void set_size(int s) { size_ = s; } + void set_capacity(int c) { capacity_ = c; } // Swaps entire contents with "other". Should be called only if the caller can // guarantee that both repeated fields are on the same arena or are on the @@ -525,83 +392,63 @@ class RepeatedField final // the old container from `old_size` to `Capacity()` (unpoison memory) // directly before it is being released, and annotate the new container from // `Capacity()` to `old_size` (poison unused memory). - void Grow(bool was_soo, int old_size, int new_size); - void GrowNoAnnotate(bool was_soo, int old_size, int new_size); + void Grow(int old_size, int new_size); + void GrowNoAnnotate(int old_size, int new_size); // Annotates a change in size of this instance. This function should be called - // with (capacity, old_size) after new memory has been allocated and filled - // from previous memory, and UnpoisonBuffer() should be called right before - // (previously annotated) memory is released. + // with (capacity, old_size) after new memory has been allocated and + // filled from previous memory), and called with (old_size, capacity) + // right before (previously annotated) memory is released. void AnnotateSize(int old_size, int new_size) const { if (old_size != new_size) { - ABSL_ATTRIBUTE_UNUSED const bool is_soo = this->is_soo(); - ABSL_ATTRIBUTE_UNUSED const Element* elem = unsafe_elements(is_soo); - ABSL_ANNOTATE_CONTIGUOUS_CONTAINER(elem, elem + Capacity(is_soo), - elem + old_size, elem + new_size); + ABSL_ANNOTATE_CONTIGUOUS_CONTAINER( + unsafe_elements(), unsafe_elements() + Capacity(), + unsafe_elements() + old_size, unsafe_elements() + new_size); if (new_size < old_size) { ABSL_ANNOTATE_MEMORY_IS_UNINITIALIZED( - elem + new_size, (old_size - new_size) * sizeof(Element)); + unsafe_elements() + new_size, + (old_size - new_size) * sizeof(Element)); } } } - // Unpoisons the memory buffer. - void UnpoisonBuffer() const { - AnnotateSize(size(), Capacity()); - if (is_soo()) { - // We need to manually unpoison the SOO buffer because in reflection for - // split repeated fields, we poison the whole SOO buffer even when we - // don't actually use the whole SOO buffer (e.g. for RepeatedField). - PROTOBUF_UNPOISON_MEMORY_REGION(soo_rep_.short_rep.data, - sizeof(soo_rep_.short_rep.data)); - } - } - - // Replaces size with new_size and returns the previous value of - // size. This function is intended to be the only place where - // size is modified, with the exception of `AddInputIterator()` - // where the size of added items is not known in advance. - inline int ExchangeCurrentSize(bool is_soo, int new_size) { - const int prev_size = size(is_soo); + // Replaces size with new_size and returns the previous value of size. This + // function is intended to be the only place where size is modified, with the + // exception of `AddInputIterator()` where the size of added items is not + // known in advance. + inline int ExchangeCurrentSize(int new_size) { + const int prev_size = size(); AnnotateSize(prev_size, new_size); - set_size(is_soo, new_size); + set_size(new_size); return prev_size; } // Returns a pointer to elements array. // pre-condition: Capacity() > 0. - Element* elements(bool is_soo) { - ABSL_DCHECK_GT(Capacity(is_soo), 0); - return unsafe_elements(is_soo); - } - const Element* elements(bool is_soo) const { - return const_cast(this)->elements(is_soo); + Element* elements() const { + ABSL_DCHECK_GT(Capacity(), 0); + // Because of above pre-condition this cast is safe. + return unsafe_elements(); } - // Returns a pointer to elements array if it exists; otherwise an invalid - // pointer is returned. This only happens for empty repeated fields, where you - // can't dereference this pointer anyway (it's empty). - Element* unsafe_elements(bool is_soo) { - return is_soo ? reinterpret_cast(soo_rep_.short_rep.data) - : reinterpret_cast(soo_rep_.long_rep.elements()); - } - const Element* unsafe_elements(bool is_soo) const { - return const_cast(this)->unsafe_elements(is_soo); + // Returns a pointer to elements array if it exists; otherwise either null or + // an invalid pointer is returned. This only happens for empty repeated + // fields, where you can't dereference this pointer anyway (it's empty). + Element* unsafe_elements() const { + return static_cast(arena_or_elements_); } // Returns a pointer to the HeapRep struct. - // pre-condition: the HeapRep must have been allocated, ie !is_soo(). + // pre-condition: the HeapRep must have been allocated, ie elements() is safe. HeapRep* heap_rep() const { - ABSL_DCHECK(!is_soo()); - return reinterpret_cast(soo_rep_.long_rep.elements() - + return reinterpret_cast(reinterpret_cast(elements()) - kHeapRepHeaderSize); } // Internal helper to delete all elements and deallocate the storage. template void InternalDeallocate() { - ABSL_DCHECK(!is_soo()); - const size_t bytes = Capacity(false) * sizeof(Element) + kHeapRepHeaderSize; + const size_t bytes = Capacity() * sizeof(Element) + kHeapRepHeaderSize; if (heap_rep()->arena == nullptr) { internal::SizedDelete(heap_rep(), bytes); } else if (!in_destructor) { @@ -621,70 +468,62 @@ class RepeatedField final // empty (common case), and add only an 8-byte header to the elements array // when non-empty. We make sure to place the size fields directly in the // RepeatedField class to avoid costly cache misses due to the indirection. - internal::SooRep soo_rep_{}; + int size_; + int capacity_; + // If capacity_ == 0 this points to an Arena otherwise it points to the + // elements member of a HeapRep struct. Using this invariant allows the + // storage of the arena pointer without an extra allocation in the + // constructor. + void* arena_or_elements_; }; // implementation ==================================================== template -constexpr RepeatedField::RepeatedField() { +constexpr RepeatedField::RepeatedField() + : size_(0), capacity_(0), arena_or_elements_(nullptr) { StaticValidityCheck(); -#ifdef __cpp_lib_is_constant_evaluated - if (!std::is_constant_evaluated()) { - AnnotateSize(kSooCapacityElements, 0); - } -#endif // __cpp_lib_is_constant_evaluated } template -inline RepeatedField::RepeatedField(Arena* arena) : soo_rep_(arena) { +inline RepeatedField::RepeatedField(Arena* arena) + : size_(0), capacity_(0), arena_or_elements_(arena) { StaticValidityCheck(); - AnnotateSize(kSooCapacityElements, 0); } template inline RepeatedField::RepeatedField(Arena* arena, const RepeatedField& rhs) - : soo_rep_(arena) { + : size_(0), capacity_(0), arena_or_elements_(arena) { StaticValidityCheck(); - AnnotateSize(kSooCapacityElements, 0); - const bool rhs_is_soo = rhs.is_soo(); - if (auto size = rhs.size(rhs_is_soo)) { - bool is_soo = true; - if (size > kSooCapacityElements) { - Grow(is_soo, 0, size); - is_soo = false; - } - ExchangeCurrentSize(is_soo, size); - UninitializedCopyN(rhs.elements(rhs_is_soo), size, unsafe_elements(is_soo)); + if (auto size = rhs.size()) { + Grow(0, size); + ExchangeCurrentSize(size); + UninitializedCopyN(rhs.elements(), size, unsafe_elements()); } } template template -RepeatedField::RepeatedField(Iter begin, Iter end) { +RepeatedField::RepeatedField(Iter begin, Iter end) + : size_(0), capacity_(0), arena_or_elements_(nullptr) { StaticValidityCheck(); - AnnotateSize(kSooCapacityElements, 0); Add(begin, end); } template RepeatedField::~RepeatedField() { StaticValidityCheck(); - const bool is_soo = this->is_soo(); #ifndef NDEBUG // Try to trigger segfault / asan failure in non-opt builds if arena_ // lifetime has ended before the destructor. - auto arena = GetArena(is_soo); + auto arena = GetArena(); if (arena) (void)arena->SpaceAllocated(); #endif - const int size = this->size(is_soo); - if (size > 0) { - Element* elem = unsafe_elements(is_soo); - Destroy(elem, elem + size); + if (Capacity() > 0) { + Destroy(unsafe_elements(), unsafe_elements() + size()); + InternalDeallocate(); } - UnpoisonBuffer(); - if (!is_soo) InternalDeallocate(); } template @@ -716,10 +555,9 @@ inline RepeatedField& RepeatedField::operator=( // We don't just call Swap(&other) here because it would perform 3 copies if // the two fields are on different arenas. if (this != &other) { - const Arena* arena = GetArena(); - if (arena != other.GetArena() + if (GetArena() != other.GetArena() #ifdef PROTOBUF_FORCE_COPY_IN_MOVE - || arena == nullptr + || GetArena() == nullptr #endif // !PROTOBUF_FORCE_COPY_IN_MOVE ) { CopyFrom(other); @@ -737,44 +575,36 @@ inline bool RepeatedField::empty() const { template inline int RepeatedField::size() const { - return size(is_soo()); + return size_; } template inline int RepeatedField::Capacity() const { - return Capacity(is_soo()); + return capacity_; } template inline void RepeatedField::AddAlreadyReserved(Element value) { - const bool is_soo = this->is_soo(); - const int old_size = size(is_soo); - ABSL_DCHECK_LT(old_size, Capacity(is_soo)); - void* p = elements(is_soo) + ExchangeCurrentSize(is_soo, old_size + 1); + ABSL_DCHECK_LT(size(), Capacity()); + void* p = elements() + ExchangeCurrentSize(size() + 1); ::new (p) Element(std::move(value)); } template inline Element* RepeatedField::AddAlreadyReserved() ABSL_ATTRIBUTE_LIFETIME_BOUND { - const bool is_soo = this->is_soo(); - const int old_size = size(is_soo); - ABSL_DCHECK_LT(old_size, Capacity(is_soo)); + ABSL_DCHECK_LT(size(), Capacity()); // new (p) compiles into nothing: this is intentional as this // function is documented to return uninitialized data for trivial types. - void* p = elements(is_soo) + ExchangeCurrentSize(is_soo, old_size + 1); + void* p = elements() + ExchangeCurrentSize(size() + 1); return ::new (p) Element; } template inline Element* RepeatedField::AddNAlreadyReserved(int n) ABSL_ATTRIBUTE_LIFETIME_BOUND { - const bool is_soo = this->is_soo(); - const int old_size = size(is_soo); - ABSL_ATTRIBUTE_UNUSED const int capacity = Capacity(is_soo); - ABSL_DCHECK_GE(capacity - old_size, n) << capacity << ", " << old_size; - Element* p = - unsafe_elements(is_soo) + ExchangeCurrentSize(is_soo, old_size + n); + ABSL_DCHECK_GE(Capacity() - size(), n) << Capacity() << ", " << size(); + Element* p = unsafe_elements() + ExchangeCurrentSize(size() + n); for (Element *begin = p, *end = p + n; begin != end; ++begin) { new (static_cast(begin)) Element; } @@ -784,20 +614,15 @@ inline Element* RepeatedField::AddNAlreadyReserved(int n) template inline void RepeatedField::Resize(int new_size, const Element& value) { ABSL_DCHECK_GE(new_size, 0); - bool is_soo = this->is_soo(); - const int old_size = size(is_soo); + const int old_size = size(); if (new_size > old_size) { - if (new_size > Capacity(is_soo)) { - Grow(is_soo, old_size, new_size); - is_soo = false; - } - Element* elem = elements(is_soo); - Element* first = elem + ExchangeCurrentSize(is_soo, new_size); - std::uninitialized_fill(first, elem + new_size, value); + if (new_size > Capacity()) Grow(old_size, new_size); + Element* first = elements() + ExchangeCurrentSize(new_size); + std::uninitialized_fill(first, elements() + new_size, value); } else if (new_size < old_size) { - Element* elem = unsafe_elements(is_soo); + Element* elem = unsafe_elements(); Destroy(elem + new_size, elem + old_size); - ExchangeCurrentSize(is_soo, new_size); + ExchangeCurrentSize(new_size); } } @@ -806,7 +631,7 @@ inline const Element& RepeatedField::Get(int index) const ABSL_ATTRIBUTE_LIFETIME_BOUND { ABSL_DCHECK_GE(index, 0); ABSL_DCHECK_LT(index, size()); - return elements(is_soo())[index]; + return elements()[index]; } template @@ -814,7 +639,7 @@ inline const Element& RepeatedField::at(int index) const ABSL_ATTRIBUTE_LIFETIME_BOUND { ABSL_CHECK_GE(index, 0); ABSL_CHECK_LT(index, size()); - return elements(is_soo())[index]; + return elements()[index]; } template @@ -822,7 +647,7 @@ inline Element& RepeatedField::at(int index) ABSL_ATTRIBUTE_LIFETIME_BOUND { ABSL_CHECK_GE(index, 0); ABSL_CHECK_LT(index, size()); - return elements(is_soo())[index]; + return elements()[index]; } template @@ -830,7 +655,7 @@ inline Element* RepeatedField::Mutable(int index) ABSL_ATTRIBUTE_LIFETIME_BOUND { ABSL_DCHECK_GE(index, 0); ABSL_DCHECK_LT(index, size()); - return &elements(is_soo())[index]; + return &elements()[index]; } template @@ -840,92 +665,69 @@ inline void RepeatedField::Set(int index, const Element& value) { template inline void RepeatedField::Add(Element value) { - bool is_soo = this->is_soo(); - const int old_size = size(is_soo); - int capacity = Capacity(is_soo); - Element* elem = unsafe_elements(is_soo); + const int old_size = size(); + int capacity = Capacity(); + Element* elem = unsafe_elements(); if (ABSL_PREDICT_FALSE(old_size == capacity)) { - Grow(is_soo, old_size, old_size + 1); - is_soo = false; - capacity = Capacity(is_soo); - elem = unsafe_elements(is_soo); + Grow(old_size, old_size + 1); + capacity = Capacity(); + elem = unsafe_elements(); } int new_size = old_size + 1; - void* p = elem + ExchangeCurrentSize(is_soo, new_size); + void* p = elem + ExchangeCurrentSize(new_size); ::new (p) Element(std::move(value)); // The below helps the compiler optimize dense loops. - // Note: we can't call functions in PROTOBUF_ASSUME so use local variables. - ABSL_ATTRIBUTE_UNUSED const int final_is_soo = this->is_soo(); - PROTOBUF_ASSUME(is_soo == final_is_soo); - ABSL_ATTRIBUTE_UNUSED const int final_size = size(is_soo); - PROTOBUF_ASSUME(new_size == final_size); - ABSL_ATTRIBUTE_UNUSED Element* const final_elements = unsafe_elements(is_soo); - PROTOBUF_ASSUME(elem == final_elements); - ABSL_ATTRIBUTE_UNUSED const int final_capacity = Capacity(is_soo); - PROTOBUF_ASSUME(capacity == final_capacity); + ABSL_ASSUME(new_size == size_); + ABSL_ASSUME(elem == arena_or_elements_); + ABSL_ASSUME(capacity == capacity_); } template inline Element* RepeatedField::Add() ABSL_ATTRIBUTE_LIFETIME_BOUND { - bool is_soo = this->is_soo(); - const int old_size = size(is_soo); + const int old_size = size(); if (ABSL_PREDICT_FALSE(old_size == Capacity())) { - Grow(is_soo, old_size, old_size + 1); - is_soo = false; + Grow(old_size, old_size + 1); } - void* p = unsafe_elements(is_soo) + ExchangeCurrentSize(is_soo, old_size + 1); + void* p = unsafe_elements() + ExchangeCurrentSize(old_size + 1); return ::new (p) Element; } template template inline void RepeatedField::AddForwardIterator(Iter begin, Iter end) { - bool is_soo = this->is_soo(); - const int old_size = size(is_soo); - int capacity = Capacity(is_soo); - Element* elem = unsafe_elements(is_soo); + const int old_size = size(); + int capacity = Capacity(); + Element* elem = unsafe_elements(); int new_size = old_size + static_cast(std::distance(begin, end)); if (ABSL_PREDICT_FALSE(new_size > capacity)) { - Grow(is_soo, old_size, new_size); - is_soo = false; - elem = unsafe_elements(is_soo); - capacity = Capacity(is_soo); + Grow(old_size, new_size); + elem = unsafe_elements(); + capacity = Capacity(); } - UninitializedCopy(begin, end, elem + ExchangeCurrentSize(is_soo, new_size)); + UninitializedCopy(begin, end, elem + ExchangeCurrentSize(new_size)); // The below helps the compiler optimize dense loops. - // Note: we can't call functions in PROTOBUF_ASSUME so use local variables. - ABSL_ATTRIBUTE_UNUSED const int final_is_soo = this->is_soo(); - PROTOBUF_ASSUME(is_soo == final_is_soo); - ABSL_ATTRIBUTE_UNUSED const int final_size = size(is_soo); - PROTOBUF_ASSUME(new_size == final_size); - ABSL_ATTRIBUTE_UNUSED Element* const final_elements = unsafe_elements(is_soo); - PROTOBUF_ASSUME(elem == final_elements); - ABSL_ATTRIBUTE_UNUSED const int final_capacity = Capacity(is_soo); - PROTOBUF_ASSUME(capacity == final_capacity); + ABSL_ASSUME(new_size == size_); + ABSL_ASSUME(elem == arena_or_elements_); + ABSL_ASSUME(capacity == capacity_); } template template inline void RepeatedField::AddInputIterator(Iter begin, Iter end) { - bool is_soo = this->is_soo(); - int size = this->size(is_soo); - int capacity = Capacity(is_soo); - Element* elem = unsafe_elements(is_soo); - Element* first = elem + size; - Element* last = elem + capacity; - UnpoisonBuffer(); + Element* elem = unsafe_elements(); + Element* first = elem + size(); + Element* last = elem + Capacity(); + AnnotateSize(size(), Capacity()); while (begin != end) { if (ABSL_PREDICT_FALSE(first == last)) { - size = first - elem; - GrowNoAnnotate(is_soo, size, size + 1); - is_soo = false; - elem = unsafe_elements(is_soo); - capacity = Capacity(is_soo); + int size = first - elem; + GrowNoAnnotate(size, size + 1); + elem = unsafe_elements(); first = elem + size; - last = elem + capacity; + last = elem + Capacity(); } ::new (static_cast(first)) Element(*begin); ++begin; @@ -933,8 +735,8 @@ inline void RepeatedField::AddInputIterator(Iter begin, Iter end) { } const int new_size = first - elem; - set_size(is_soo, new_size); - AnnotateSize(capacity, new_size); + set_size(new_size); + AnnotateSize(Capacity(), new_size); } template @@ -951,11 +753,10 @@ inline void RepeatedField::Add(Iter begin, Iter end) { template inline void RepeatedField::RemoveLast() { - const bool is_soo = this->is_soo(); - const int old_size = size(is_soo); + const int old_size = size(); ABSL_DCHECK_GT(old_size, 0); - elements(is_soo)[old_size - 1].~Element(); - ExchangeCurrentSize(is_soo, old_size - 1); + elements()[old_size - 1].~Element(); + ExchangeCurrentSize(old_size - 1); } template @@ -963,10 +764,9 @@ void RepeatedField::ExtractSubrange(int start, int num, Element* elements) { ABSL_DCHECK_GE(start, 0); ABSL_DCHECK_GE(num, 0); - const bool is_soo = this->is_soo(); - const int old_size = size(is_soo); + const int old_size = size(); ABSL_DCHECK_LE(start + num, old_size); - Element* elem = unsafe_elements(is_soo); + Element* elem = unsafe_elements(); // Save the values of the removed elements if requested. if (elements != nullptr) { @@ -983,23 +783,19 @@ void RepeatedField::ExtractSubrange(int start, int num, template inline void RepeatedField::Clear() { - const bool is_soo = this->is_soo(); - Element* elem = unsafe_elements(is_soo); - Destroy(elem, elem + size(is_soo)); - ExchangeCurrentSize(is_soo, 0); + Element* elem = unsafe_elements(); + Destroy(elem, elem + size()); + ExchangeCurrentSize(0); } template inline void RepeatedField::MergeFrom(const RepeatedField& other) { ABSL_DCHECK_NE(&other, this); - const bool other_is_soo = other.is_soo(); - if (auto other_size = other.size(other_is_soo)) { + if (auto other_size = other.size()) { const int old_size = size(); Reserve(old_size + other_size); - const bool is_soo = this->is_soo(); - Element* dst = - elements(is_soo) + ExchangeCurrentSize(is_soo, old_size + other_size); - UninitializedCopyN(other.elements(other_is_soo), other_size, dst); + Element* dst = elements() + ExchangeCurrentSize(old_size + other_size); + UninitializedCopyN(other.elements(), other_size, dst); } } @@ -1036,13 +832,13 @@ inline typename RepeatedField::iterator RepeatedField::erase( template inline Element* RepeatedField::mutable_data() ABSL_ATTRIBUTE_LIFETIME_BOUND { - return unsafe_elements(is_soo()); + return unsafe_elements(); } template inline const Element* RepeatedField::data() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - return unsafe_elements(is_soo()); + return unsafe_elements(); } template @@ -1050,31 +846,27 @@ inline void RepeatedField::InternalSwap( RepeatedField* PROTOBUF_RESTRICT other) { ABSL_DCHECK(this != other); - // We need to unpoison during the swap in case we're in SOO mode. - UnpoisonBuffer(); - other->UnpoisonBuffer(); - - internal::memswap( - reinterpret_cast(&this->soo_rep_), - reinterpret_cast(&other->soo_rep_)); - - AnnotateSize(Capacity(), size()); - other->AnnotateSize(other->Capacity(), other->size()); + // Swap all fields at once. + static_assert(std::is_standard_layout>::value, + "offsetof() requires standard layout before c++17"); + static constexpr size_t kOffset = offsetof(RepeatedField, size_); + internal::memswaparena_or_elements_) - kOffset>( + reinterpret_cast(this) + kOffset, + reinterpret_cast(other) + kOffset); } template void RepeatedField::Swap(RepeatedField* other) { if (this == other) return; - Arena* arena = GetArena(); - Arena* other_arena = other->GetArena(); #ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (arena != nullptr && arena == other_arena) { + if (GetArena() != nullptr && GetArena() == other->GetArena()) { #else // PROTOBUF_FORCE_COPY_IN_SWAP - if (arena == other_arena) { + if (GetArena() == other->GetArena()) { #endif // !PROTOBUF_FORCE_COPY_IN_SWAP InternalSwap(other); } else { - RepeatedField temp(other_arena); + RepeatedField temp(other->GetArena()); temp.MergeFrom(*this); CopyFrom(*other); other->UnsafeArenaSwap(&temp); @@ -1090,51 +882,45 @@ void RepeatedField::UnsafeArenaSwap(RepeatedField* other) { template void RepeatedField::SwapElements(int index1, int index2) { - Element* elem = elements(is_soo()); using std::swap; // enable ADL with fallback - swap(elem[index1], elem[index2]); + swap(elements()[index1], elements()[index2]); } template inline typename RepeatedField::iterator RepeatedField::begin() ABSL_ATTRIBUTE_LIFETIME_BOUND { - return iterator(unsafe_elements(is_soo())); + return iterator(unsafe_elements()); } template inline typename RepeatedField::const_iterator RepeatedField::begin() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - return const_iterator(unsafe_elements(is_soo())); + return const_iterator(unsafe_elements()); } template inline typename RepeatedField::const_iterator RepeatedField::cbegin() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - return const_iterator(unsafe_elements(is_soo())); + return const_iterator(unsafe_elements()); } template inline typename RepeatedField::iterator RepeatedField::end() ABSL_ATTRIBUTE_LIFETIME_BOUND { - const bool is_soo = this->is_soo(); - return iterator(unsafe_elements(is_soo) + size(is_soo)); + return iterator(unsafe_elements() + size()); } template inline typename RepeatedField::const_iterator RepeatedField::end() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - const bool is_soo = this->is_soo(); - return const_iterator(unsafe_elements(is_soo) + size(is_soo)); + return const_iterator(unsafe_elements() + size()); } template inline typename RepeatedField::const_iterator RepeatedField::cend() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - const bool is_soo = this->is_soo(); - return const_iterator(unsafe_elements(is_soo) + size(is_soo)); + return const_iterator(unsafe_elements() + size()); } template inline size_t RepeatedField::SpaceUsedExcludingSelfLong() const { const int capacity = Capacity(); - return capacity > kSooCapacityElements - ? capacity * sizeof(Element) + kHeapRepHeaderSize - : 0; + return capacity > 0 ? capacity * sizeof(Element) + kHeapRepHeaderSize : 0; } namespace internal { @@ -1142,7 +928,10 @@ namespace internal { // requested 'new_size'. The result is clamped to the closed interval: // [internal::kMinRepeatedFieldAllocationSize, // std::numeric_limits::max()] -// Requires: new_size > capacity +// Requires: +// new_size > capacity && +// (capacity == 0 || +// capacity >= kRepeatedFieldLowerClampLimit) template inline int CalculateReserveSize(int capacity, int new_size) { constexpr int lower_limit = @@ -1156,15 +945,6 @@ inline int CalculateReserveSize(int capacity, int new_size) { if (PROTOBUF_PREDICT_FALSE(capacity > kMaxSizeBeforeClamp)) { return std::numeric_limits::max(); } - constexpr int kSooCapacityElements = SooCapacityElements(sizeof(T)); - if (kSooCapacityElements > 0 && kSooCapacityElements < lower_limit) { - // In this case, we need to set capacity to 0 here to ensure power-of-two - // sized allocations. - if (capacity < lower_limit) capacity = 0; - } else { - ABSL_DCHECK(capacity == 0 || capacity >= lower_limit) - << capacity << " " << lower_limit; - } // We want to double the number of bytes, not the number of elements, to try // to stay within power-of-two allocations. // The allocation has kHeapRepHeaderSize + sizeof(T) * capacity. @@ -1175,25 +955,22 @@ inline int CalculateReserveSize(int capacity, int new_size) { template void RepeatedField::Reserve(int new_size) { - const bool was_soo = is_soo(); - if (ABSL_PREDICT_FALSE(new_size > Capacity(was_soo))) { - Grow(was_soo, size(was_soo), new_size); + if (ABSL_PREDICT_FALSE(new_size > Capacity())) { + Grow(size(), new_size); } } // Avoid inlining of Reserve(): new, copy, and delete[] lead to a significant // amount of code bloat. template -PROTOBUF_NOINLINE void RepeatedField::GrowNoAnnotate(bool was_soo, - int old_size, +PROTOBUF_NOINLINE void RepeatedField::GrowNoAnnotate(int old_size, int new_size) { - const int old_capacity = Capacity(was_soo); - ABSL_DCHECK_GT(new_size, old_capacity); + ABSL_DCHECK_GT(new_size, Capacity()); HeapRep* new_rep; Arena* arena = GetArena(); new_size = internal::CalculateReserveSize( - old_capacity, new_size); + Capacity(), new_size); ABSL_DCHECK_LE(static_cast(new_size), (std::numeric_limits::max() - kHeapRepHeaderSize) / @@ -1217,22 +994,25 @@ PROTOBUF_NOINLINE void RepeatedField::GrowNoAnnotate(bool was_soo, } new_rep->arena = arena; - if (old_size > 0) { - Element* pnew = static_cast(new_rep->elements()); - Element* pold = elements(was_soo); - // TODO: add absl::is_trivially_relocatable - if (std::is_trivial::value) { - memcpy(static_cast(pnew), pold, old_size * sizeof(Element)); - } else { - for (Element* end = pnew + old_size; pnew != end; ++pnew, ++pold) { - ::new (static_cast(pnew)) Element(std::move(*pold)); - pold->~Element(); + if (Capacity() > 0) { + if (old_size > 0) { + Element* pnew = static_cast(new_rep->elements()); + Element* pold = elements(); + // TODO: add absl::is_trivially_relocatable + if (std::is_trivial::value) { + memcpy(static_cast(pnew), pold, old_size * sizeof(Element)); + } else { + for (Element* end = pnew + old_size; pnew != end; ++pnew, ++pold) { + ::new (static_cast(pnew)) Element(std::move(*pold)); + pold->~Element(); + } } } + InternalDeallocate(); } - if (!was_soo) InternalDeallocate(); - soo_rep_.set_non_soo(was_soo, new_size, new_rep->elements()); + set_capacity(new_size); + arena_or_elements_ = static_cast(new_rep->elements()); } // Ideally we would be able to use: @@ -1241,22 +1021,21 @@ PROTOBUF_NOINLINE void RepeatedField::GrowNoAnnotate(bool was_soo, // However, as explained in b/266411038#comment9, this causes issues // in shared libraries for Youtube (and possibly elsewhere). template -PROTOBUF_NOINLINE void RepeatedField::Grow(bool was_soo, int old_size, +PROTOBUF_NOINLINE void RepeatedField::Grow(int old_size, int new_size) { - UnpoisonBuffer(); - GrowNoAnnotate(was_soo, old_size, new_size); + AnnotateSize(old_size, Capacity()); + GrowNoAnnotate(old_size, new_size); AnnotateSize(Capacity(), old_size); } template inline void RepeatedField::Truncate(int new_size) { - const bool is_soo = this->is_soo(); - const int old_size = size(is_soo); + const int old_size = size(); ABSL_DCHECK_LE(new_size, old_size); if (new_size < old_size) { - Element* elem = unsafe_elements(is_soo); + Element* elem = unsafe_elements(); Destroy(elem + new_size, elem + old_size); - ExchangeCurrentSize(is_soo, new_size); + ExchangeCurrentSize(new_size); } } diff --git a/src/google/protobuf/repeated_field_unittest.cc b/src/google/protobuf/repeated_field_unittest.cc index 574db69560..50e43f594e 100644 --- a/src/google/protobuf/repeated_field_unittest.cc +++ b/src/google/protobuf/repeated_field_unittest.cc @@ -201,10 +201,12 @@ TEST(RepeatedField, Small) { EXPECT_TRUE(field.empty()); EXPECT_EQ(field.size(), 0); - if (sizeof(void*) == 8) { - // Usage should be 0 because this should fit in SOO space. - EXPECT_EQ(field.SpaceUsedExcludingSelf(), 0); - } + // Additional bytes are for 'struct Rep' header. + int expected_usage = + (sizeof(Arena*) > sizeof(int) ? sizeof(Arena*) / sizeof(int) : 3) * + sizeof(int) + + sizeof(Arena*); + EXPECT_GE(field.SpaceUsedExcludingSelf(), expected_usage); } @@ -256,11 +258,8 @@ void CheckAllocationSizes(bool is_ptr) { ASSERT_EQ((1 << log2), last_alloc); } - // The byte size must be a multiple of 8 when not SOO. - const int capacity_bytes = rep->Capacity() * sizeof(T); - if (capacity_bytes > internal::kSooCapacityBytes) { - ASSERT_EQ(capacity_bytes % 8, 0); - } + // The byte size must be a multiple of 8. + ASSERT_EQ(rep->Capacity() * sizeof(T) % 8, 0); } } } @@ -482,6 +481,14 @@ TEST(RepeatedField, Resize) { EXPECT_TRUE(field.empty()); } +TEST(RepeatedField, ReserveNothing) { + RepeatedField field; + EXPECT_EQ(0, field.Capacity()); + + field.Reserve(-1); + EXPECT_EQ(0, field.Capacity()); +} + TEST(RepeatedField, ReserveLowerClamp) { int clamped_value = internal::CalculateReserveSize(0, 1); EXPECT_GE(clamped_value, sizeof(void*) / sizeof(bool)); @@ -892,7 +899,9 @@ TEST(RepeatedField, MoveConstruct) { RepeatedField source; source.Add(1); source.Add(2); + const int* data = source.data(); RepeatedField destination = std::move(source); + EXPECT_EQ(data, destination.data()); EXPECT_THAT(destination, ElementsAre(1, 2)); // This property isn't guaranteed but it's useful to have a test that would // catch changes in this area. @@ -919,8 +928,14 @@ TEST(RepeatedField, MoveAssign) { source.Add(2); RepeatedField destination; destination.Add(3); + const int* source_data = source.data(); + const int* destination_data = destination.data(); destination = std::move(source); + EXPECT_EQ(source_data, destination.data()); EXPECT_THAT(destination, ElementsAre(1, 2)); + // This property isn't guaranteed but it's useful to have a test that would + // catch changes in this area. + EXPECT_EQ(destination_data, source.data()); EXPECT_THAT(source, ElementsAre(3)); } { @@ -930,7 +945,9 @@ TEST(RepeatedField, MoveAssign) { source->Add(2); RepeatedField* destination = Arena::Create>(&arena); destination->Add(3); + const int* source_data = source->data(); *destination = std::move(*source); + EXPECT_EQ(source_data, destination->data()); EXPECT_THAT(*destination, ElementsAre(1, 2)); EXPECT_THAT(*source, ElementsAre(3)); } @@ -982,7 +999,9 @@ TEST(RepeatedField, MoveAssign) { RepeatedField& alias = field; field.Add(1); field.Add(2); + const int* data = field.data(); field = std::move(alias); + EXPECT_EQ(data, field.data()); EXPECT_THAT(field, ElementsAre(1, 2)); } { @@ -990,7 +1009,9 @@ TEST(RepeatedField, MoveAssign) { RepeatedField* field = Arena::Create>(&arena); field->Add(1); field->Add(2); + const int* data = field->data(); *field = std::move(*field); + EXPECT_EQ(data, field->data()); EXPECT_THAT(*field, ElementsAre(1, 2)); } } @@ -1325,20 +1346,6 @@ TEST(RepeatedField, Cleanups) { EXPECT_THAT(growth.cleanups, testing::UnorderedElementsAre(ptr)); } -TEST(RepeatedField, InitialSooCapacity) { - if (sizeof(void*) == 8) { - EXPECT_EQ(RepeatedField().Capacity(), 3); - EXPECT_EQ(RepeatedField().Capacity(), 2); - EXPECT_EQ(RepeatedField().Capacity(), 1); - EXPECT_EQ(RepeatedField().Capacity(), 0); - } else { - EXPECT_EQ(RepeatedField().Capacity(), 1); - EXPECT_EQ(RepeatedField().Capacity(), 1); - EXPECT_EQ(RepeatedField().Capacity(), 1); - EXPECT_EQ(RepeatedField().Capacity(), 0); - } -} - // =================================================================== // RepeatedPtrField tests. These pretty much just mirror the RepeatedField // tests above.