From 91e31622404f1340c34bd1c1bfe06938b954e5ca Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 12 Sep 2024 17:03:26 -0700 Subject: [PATCH] Streamline creation of new elements in `RepeatedPtrField` The logic to create new elements in `RepeatedPtrField` was spread across an assorted collection of specializations of `GenericTypeHandler`, individual `GenericTypeHandler` static functions, and external member and non-member functions, all living in multiple sources. Now all that logic is concentrated in three symmetric `GenericTypeHandler` specializations. PiperOrigin-RevId: 674064544 --- src/google/protobuf/extension_set.cc | 2 +- src/google/protobuf/lazy_repeated_field.h | 2 +- src/google/protobuf/message.cc | 22 ---- src/google/protobuf/message_lite.cc | 10 -- src/google/protobuf/repeated_ptr_field.cc | 17 --- src/google/protobuf/repeated_ptr_field.h | 133 +++++++++------------- 6 files changed, 58 insertions(+), 128 deletions(-) diff --git a/src/google/protobuf/extension_set.cc b/src/google/protobuf/extension_set.cc index 34a13fe5039e..12a730ce1ca3 100644 --- a/src/google/protobuf/extension_set.cc +++ b/src/google/protobuf/extension_set.cc @@ -825,7 +825,7 @@ MessageLite* ExtensionSet::AddMessage(int number, FieldType type, return reinterpret_cast( extension->ptr.repeated_message_value) - ->AddMessage(&prototype); + ->AddFromPrototype(&prototype); } // Defined in extension_set_heavy.cc. diff --git a/src/google/protobuf/lazy_repeated_field.h b/src/google/protobuf/lazy_repeated_field.h index ab573e58c081..c166baaed329 100644 --- a/src/google/protobuf/lazy_repeated_field.h +++ b/src/google/protobuf/lazy_repeated_field.h @@ -328,7 +328,7 @@ class LazyRepeatedPtrField { const char* ptr2 = ptr; TagType next_tag; do { - MessageLite* submsg = value->AddMessage(prototype); + MessageLite* submsg = value->AddFromPrototype(prototype); // ptr2 points to the start of the element's encoded length. ptr = ctx->ParseMessage(submsg, ptr2); if (PROTOBUF_PREDICT_FALSE(ptr == nullptr)) return nullptr; diff --git a/src/google/protobuf/message.cc b/src/google/protobuf/message.cc index d265e284e10f..f326125e851a 100644 --- a/src/google/protobuf/message.cc +++ b/src/google/protobuf/message.cc @@ -495,28 +495,6 @@ const internal::RepeatedFieldAccessor* Reflection::RepeatedFieldAccessor( } namespace internal { -template <> -#if defined(_MSC_VER) && (_MSC_VER >= 1800) -// Note: force noinline to workaround MSVC compiler bug with /Zc:inline, issue -// #240 -PROTOBUF_NOINLINE -#endif - Message* - GenericTypeHandler::NewFromPrototype(const Message* prototype, - Arena* arena) { - return prototype->New(arena); -} -template <> -#if defined(_MSC_VER) && (_MSC_VER >= 1800) -// Note: force noinline to workaround MSVC compiler bug with /Zc:inline, issue -// #240 -PROTOBUF_NOINLINE -#endif - Arena* - GenericTypeHandler::GetArena(Message* value) { - return value->GetArena(); -} - template void InternalMetadata::DoClear(); template void InternalMetadata::DoMergeFrom( const UnknownFieldSet& other); diff --git a/src/google/protobuf/message_lite.cc b/src/google/protobuf/message_lite.cc index 456b0b063e71..e7bd95904787 100644 --- a/src/google/protobuf/message_lite.cc +++ b/src/google/protobuf/message_lite.cc @@ -711,16 +711,6 @@ absl::Cord MessageLite::SerializePartialAsCord() const { namespace internal { -MessageLite* NewFromPrototypeHelper(const MessageLite* prototype, - Arena* arena) { - return prototype->New(arena); -} -template <> -void GenericTypeHandler::Merge(const MessageLite& from, - MessageLite* to) { - to->CheckTypeAndMergeFrom(from); -} - // Non-inline variants of std::string specializations for // various InternalMetadata routines. template <> diff --git a/src/google/protobuf/repeated_ptr_field.cc b/src/google/protobuf/repeated_ptr_field.cc index 5a899210c0a1..4c847866cb4c 100644 --- a/src/google/protobuf/repeated_ptr_field.cc +++ b/src/google/protobuf/repeated_ptr_field.cc @@ -93,14 +93,6 @@ void RepeatedPtrFieldBase::DestroyProtos() { tagged_rep_or_elem_ = nullptr; } -void* RepeatedPtrFieldBase::AddMessageLite(ElementFactory factory) { - return AddInternal(factory); -} - -void* RepeatedPtrFieldBase::AddString() { - return AddInternal([](Arena* arena) { return NewStringElement(arena); }); -} - void RepeatedPtrFieldBase::CloseGap(int start, int num) { if (using_sso()) { if (start == 0 && num == 1) { @@ -116,11 +108,6 @@ void RepeatedPtrFieldBase::CloseGap(int start, int num) { ExchangeCurrentSize(current_size_ - num); } -MessageLite* RepeatedPtrFieldBase::AddMessage(const MessageLite* prototype) { - return static_cast( - AddInternal([prototype](Arena* a) { return prototype->New(a); })); -} - void InternalOutOfLineDeleteMessageLite(MessageLite* message) { delete message; } @@ -227,10 +214,6 @@ void RepeatedPtrFieldBase::MergeFrom( } } -void* NewStringElement(Arena* arena) { - return Arena::Create(arena); -} - } // 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 10c57127dab8..1a0e6126b358 100644 --- a/src/google/protobuf/repeated_ptr_field.h +++ b/src/google/protobuf/repeated_ptr_field.h @@ -108,7 +108,8 @@ class GenericTypeHandler; // using Type = MyType; // using Movable = ...; // -// static Type*(*)(Arena*) GetNewFunc(); +// static ElementFactory GetNewFunc(); +// static ElementFactory GetNewFromPrototypeFunc(const Type* prototype); // static void GetArena(Type* value); // // static Type* New(Arena* arena); @@ -127,8 +128,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { static constexpr int kSSOCapacity = 1; - using ElementFactory = void* (*)(Arena*); - protected: // We use the same TypeHandler for all Message types to deduplicate generated // code. @@ -192,15 +191,18 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { template Value* Add() { - if (std::is_same, std::string>{}) { - return cast(AddString()); - } - return cast(AddMessageLite(TypeHandler::GetNewFunc())); + return cast(AddInternal(TypeHandler::GetNewFunc())); } - template < - typename TypeHandler, - typename std::enable_if::type* = nullptr> + template + Type* AddFromPrototype(const Type* prototype) { + using TypeHandler = GenericTypeHandler; + using H = CommonHandler; + return cast( + AddInternal(H::GetNewFromPrototypeFunc(prototype))); + } + + template inline void Add(Value&& value) { if (current_size_ < allocated_size()) { *cast(element_at(ExchangeCurrentSize(current_size_ + 1))) = @@ -255,12 +257,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { return *cast(element_at(index)); } - // Creates and adds an element using the given prototype, without introducing - // a link-time dependency on the concrete message type. - // - // Pre-condition: prototype must not be nullptr. - MessageLite* AddMessage(const MessageLite* prototype); - template void Clear() { const int n = current_size_; @@ -725,10 +721,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { return InternalExtend(n - Capacity()); } - // Internal helpers for Add that keep definition out-of-line. - void* AddMessageLite(ElementFactory factory); - void* AddString(); - // Common implementation used by various Add* methods. `factory` is an object // used to construct a new element unless there are spare cleared elements // ready for reuse. Returns pointer to the new element. @@ -813,13 +805,32 @@ void* RepeatedPtrFieldBase::AddInternal(Factory factory) { PROTOBUF_EXPORT void InternalOutOfLineDeleteMessageLite(MessageLite* message); +// Encapsulates the minimally required subset of T's properties in a +// `RepeatedPtrField` specialization so the type-agnostic +// `RepeatedPtrFieldBase` could do its job without knowing T. +// +// This generic definition is for types derived from `MessageLite`. That is +// statically asserted, but only where a non-conforming type would emit a +// compile-time diagnostic that lacks proper guidance for fixing. Asserting +// at the top level isn't possible, because some template argument types are not +// yet fully defined at the instantiation point. +// +// Explicit specializations are provided for `std::string` and +// `StringPieceField` further below. template class GenericTypeHandler { public: using Type = GenericType; using Movable = IsMovable; - static constexpr auto GetNewFunc() { return Arena::DefaultConstruct; } + static constexpr auto* GetNewFunc() { return Arena::DefaultConstruct; } + static constexpr auto GetNewFromPrototypeFunc(const Type* prototype) { + static_assert(std::is_base_of::value, ""); + return [prototype](Arena* arena) { + ABSL_DCHECK(prototype != nullptr); + return prototype->New(arena); + }; + } static inline Arena* GetArena(Type* value) { return Arena::InternalGetArena(value); } @@ -830,76 +841,42 @@ class GenericTypeHandler { static inline Type* New(Arena* arena, Type&& value) { return Arena::Create(arena, std::move(value)); } - static inline Type* NewFromPrototype(const Type* /*prototype*/, + static inline Type* NewFromPrototype(const Type* prototype, Arena* arena = nullptr) { - return New(arena); + static_assert(std::is_base_of::value, ""); + ABSL_DCHECK(prototype != nullptr); + return prototype->New(arena); } static inline void Delete(Type* value, Arena* arena) { if (arena != nullptr) return; -#ifdef __cpp_if_constexpr - if constexpr (std::is_base_of::value) { - // Using virtual destructor to reduce generated code size that would have - // happened otherwise due to inlined `~Type()`. - InternalOutOfLineDeleteMessageLite(value); - } else { - delete value; - } -#else - delete value; -#endif + // Using virtual destructor to reduce generated code size that would have + // happened otherwise due to inlined `~Type()`. + InternalOutOfLineDeleteMessageLite(value); + } + static inline void Clear(Type* value) { + static_assert(std::is_base_of::value, ""); + value->Clear(); + } + static inline void Merge(const Type& from, Type* to) { + static_assert(std::is_base_of::value, ""); + to->CheckTypeAndMergeFrom(from); } - static inline void Clear(Type* value) { value->Clear(); } - static void Merge(const Type& from, Type* to); static inline size_t SpaceUsedLong(const Type& value) { + static_assert(std::is_base_of::value, ""); return value.SpaceUsedLong(); } }; -// NewFromPrototypeHelper() is not defined inline here, as we will need to do a -// virtual function dispatch anyways to go from Message* to call New/Merge. (The -// additional helper is needed as a workaround for MSVC.) -PROTOBUF_EXPORT MessageLite* NewFromPrototypeHelper( - const MessageLite* prototype, Arena* arena); - -template <> -inline MessageLite* GenericTypeHandler::NewFromPrototype( - const MessageLite* prototype, Arena* arena) { - return NewFromPrototypeHelper(prototype, arena); -} -template <> -inline Arena* GenericTypeHandler::GetArena(MessageLite* value) { - return value->GetArena(); -} - -template -PROTOBUF_NOINLINE inline void GenericTypeHandler::Merge( - const GenericType& from, GenericType* to) { - to->MergeFrom(from); -} -template <> -PROTOBUF_EXPORT void GenericTypeHandler::Merge( - const MessageLite& from, MessageLite* to); - -// Message specialization bodies defined in message.cc. This split is necessary -// to allow proto2-lite (which includes this header) to be independent of -// Message. -template <> -PROTOBUF_EXPORT Message* GenericTypeHandler::NewFromPrototype( - const Message* prototype, Arena* arena); -template <> -PROTOBUF_EXPORT Arena* GenericTypeHandler::GetArena(Message* value); - -PROTOBUF_EXPORT void* NewStringElement(Arena* arena); - template <> class GenericTypeHandler { public: using Type = std::string; using Movable = IsMovable; - static constexpr auto GetNewFunc() { return NewStringElement; } - static inline Arena* GetArena(Type*) { return nullptr; } - + static constexpr auto* GetNewFunc() { return Arena::Create; } + static constexpr auto* GetNewFromPrototypeFunc(const Type* prototype) { + return GetNewFunc(); + } static PROTOBUF_NOINLINE Type* New(Arena* arena) { return Arena::Create(arena); } @@ -914,13 +891,15 @@ class GenericTypeHandler { delete value; } } + static inline Arena* GetArena(Type*) { return nullptr; } static inline void Clear(Type* value) { value->clear(); } static inline void Merge(const Type& from, Type* to) { *to = from; } - static size_t SpaceUsedLong(const Type& value) { + static inline size_t SpaceUsedLong(const Type& value) { return sizeof(value) + StringSpaceUsedExcludingSelfLong(value); } }; + } // namespace internal // RepeatedPtrField is like RepeatedField, but used for repeated strings or @@ -1246,7 +1225,7 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase { void AddAllocatedForParse(Element* p) { - return RepeatedPtrFieldBase::AddAllocatedForParse(p); + RepeatedPtrFieldBase::AddAllocatedForParse(p); } };