From b0d7441f83a4cc79d3937f65f8cc2b46931ea639 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 16 Nov 2023 15:42:45 -0800 Subject: [PATCH] Avoid calling GetArena() multiple times. PiperOrigin-RevId: 583188151 --- .../cpp/field_generators/cord_field.cc | 15 ++-- src/google/protobuf/dynamic_message.cc | 19 ++-- src/google/protobuf/extension_set.cc | 88 ++++++++++--------- .../protobuf/generated_message_reflection.cc | 81 +++++++++-------- src/google/protobuf/repeated_ptr_field.cc | 7 +- 5 files changed, 116 insertions(+), 94 deletions(-) diff --git a/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc b/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc index 62cbbae303..732a60c098 100644 --- a/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc +++ b/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc @@ -352,8 +352,9 @@ void CordOneofFieldGenerator::GenerateInlineAccessorDefinitions( clear_$oneof_name$(); set_has_$name$(); $field$ = new ::absl::Cord; - if (GetArena() != nullptr) { - GetArena()->Own($field$); + ::$proto_ns$::Arena* arena = GetArena(); + if (arena != nullptr) { + arena->Own($field$); } } *$field$ = value; @@ -372,8 +373,9 @@ void CordOneofFieldGenerator::GenerateInlineAccessorDefinitions( clear_$oneof_name$(); set_has_$name$(); $field$ = new ::absl::Cord; - if (GetArena() != nullptr) { - GetArena()->Own($field$); + ::$proto_ns$::Arena* arena = GetArena(); + if (arena != nullptr) { + arena->Own($field$); } } *$field$ = value; @@ -387,8 +389,9 @@ void CordOneofFieldGenerator::GenerateInlineAccessorDefinitions( clear_$oneof_name$(); set_has_$name$(); $field$ = new ::absl::Cord; - if (GetArena() != nullptr) { - GetArena()->Own($field$); + ::$proto_ns$::Arena* arena = GetArena(); + if (arena != nullptr) { + arena->Own($field$); } } return $field$; diff --git a/src/google/protobuf/dynamic_message.cc b/src/google/protobuf/dynamic_message.cc index 94d7e45bd0..7872866dfc 100644 --- a/src/google/protobuf/dynamic_message.cc +++ b/src/google/protobuf/dynamic_message.cc @@ -351,6 +351,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { // constructor.) const Descriptor* descriptor = type_info_->type; + Arena* arena = GetArena(); // Initialize oneof cases. int oneof_count = 0; for (int i = 0; i < descriptor->oneof_decl_count(); ++i) { @@ -360,7 +361,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { } if (type_info_->extensions_offset != -1) { - new (MutableExtensionsRaw()) ExtensionSet(GetArena()); + new (MutableExtensionsRaw()) ExtensionSet(arena); } for (int i = 0; i < descriptor->field_count(); i++) { const FieldDescriptor* field = descriptor->field(i); @@ -374,7 +375,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { if (!field->is_repeated()) { \ new (field_ptr) TYPE(field->default_value_##TYPE()); \ } else { \ - new (field_ptr) RepeatedField(GetArena()); \ + new (field_ptr) RepeatedField(arena); \ } \ break; @@ -391,7 +392,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { if (!field->is_repeated()) { new (field_ptr) int{field->default_value_enum()->number()}; } else { - new (field_ptr) RepeatedField(GetArena()); + new (field_ptr) RepeatedField(arena); } break; @@ -403,7 +404,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { ArenaStringPtr* asp = new (field_ptr) ArenaStringPtr(); asp->InitDefault(); } else { - new (field_ptr) RepeatedPtrField(GetArena()); + new (field_ptr) RepeatedPtrField(arena); } break; } @@ -418,20 +419,20 @@ void DynamicMessage::SharedCtor(bool lock_factory) { // when the constructor is called inside GetPrototype(), in which // case we have already locked the factory. if (lock_factory) { - if (GetArena() != nullptr) { + if (arena != nullptr) { new (field_ptr) DynamicMapField( type_info_->factory->GetPrototype(field->message_type()), - GetArena()); + arena); } else { new (field_ptr) DynamicMapField( type_info_->factory->GetPrototype(field->message_type())); } } else { - if (GetArena() != nullptr) { + if (arena != nullptr) { new (field_ptr) DynamicMapField(type_info_->factory->GetPrototypeNoLock( field->message_type()), - GetArena()); + arena); } else { new (field_ptr) DynamicMapField(type_info_->factory->GetPrototypeNoLock( @@ -439,7 +440,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { } } } else { - new (field_ptr) RepeatedPtrField(GetArena()); + new (field_ptr) RepeatedPtrField(arena); } } break; diff --git a/src/google/protobuf/extension_set.cc b/src/google/protobuf/extension_set.cc index 32528d4cb2..0a67b05991 100644 --- a/src/google/protobuf/extension_set.cc +++ b/src/google/protobuf/extension_set.cc @@ -630,38 +630,40 @@ void ExtensionSet::SetAllocatedMessage(int number, FieldType type, ClearExtension(number); return; } - ABSL_DCHECK(message->GetArena() == nullptr || message->GetArena() == arena_); - Arena* message_arena = message->GetArena(); + Arena* const arena = arena_; + Arena* const message_arena = message->GetArena(); + ABSL_DCHECK(message_arena == nullptr || message_arena == arena); + Extension* extension; if (MaybeNewExtension(number, descriptor, &extension)) { extension->type = type; ABSL_DCHECK_EQ(cpp_type(extension->type), WireFormatLite::CPPTYPE_MESSAGE); extension->is_repeated = false; extension->is_lazy = false; - if (message_arena == arena_) { + if (message_arena == arena) { extension->message_value = message; } else if (message_arena == nullptr) { extension->message_value = message; - arena_->Own(message); // not nullptr because not equal to message_arena + arena->Own(message); // not nullptr because not equal to message_arena } else { - extension->message_value = message->New(arena_); + extension->message_value = message->New(arena); extension->message_value->CheckTypeAndMergeFrom(*message); } } else { ABSL_DCHECK_TYPE(*extension, OPTIONAL_FIELD, MESSAGE); if (extension->is_lazy) { - extension->lazymessage_value->SetAllocatedMessage(message, arena_); + extension->lazymessage_value->SetAllocatedMessage(message, arena); } else { - if (arena_ == nullptr) { + if (arena == nullptr) { delete extension->message_value; } - if (message_arena == arena_) { + if (message_arena == arena) { extension->message_value = message; } else if (message_arena == nullptr) { extension->message_value = message; - arena_->Own(message); // not nullptr because not equal to message_arena + arena->Own(message); // not nullptr because not equal to message_arena } else { - extension->message_value = message->New(arena_); + extension->message_value = message->New(arena); extension->message_value->CheckTypeAndMergeFrom(*message); } } @@ -708,8 +710,9 @@ MessageLite* ExtensionSet::ReleaseMessage(int number, ABSL_DCHECK_TYPE(*extension, OPTIONAL_FIELD, MESSAGE); MessageLite* ret = nullptr; if (extension->is_lazy) { - ret = extension->lazymessage_value->ReleaseMessage(prototype, arena_); - if (arena_ == nullptr) { + Arena* const arena = arena_; + ret = extension->lazymessage_value->ReleaseMessage(prototype, arena); + if (arena == nullptr) { delete extension->lazymessage_value; } } else { @@ -737,9 +740,10 @@ MessageLite* ExtensionSet::UnsafeArenaReleaseMessage( ABSL_DCHECK_TYPE(*extension, OPTIONAL_FIELD, MESSAGE); MessageLite* ret = nullptr; if (extension->is_lazy) { + Arena* const arena = arena_; ret = extension->lazymessage_value->UnsafeArenaReleaseMessage(prototype, - arena_); - if (arena_ == nullptr) { + arena); + if (arena == nullptr) { delete extension->lazymessage_value; } } else { @@ -994,10 +998,11 @@ void ExtensionSet::InternalExtensionMergeFrom(const MessageLite* extendee, HANDLE_TYPE(STRING, string, RepeatedPtrField); #undef HANDLE_TYPE - case WireFormatLite::CPPTYPE_MESSAGE: + case WireFormatLite::CPPTYPE_MESSAGE: { + Arena* const arena = arena_; if (is_new) { extension->repeated_message_value = - Arena::CreateMessage>(arena_); + Arena::CreateMessage>(arena); } // We can't call RepeatedPtrField::MergeFrom() because // it would attempt to allocate new objects. @@ -1010,12 +1015,13 @@ void ExtensionSet::InternalExtensionMergeFrom(const MessageLite* extendee, extension->repeated_message_value) ->AddFromCleared>(); if (target == nullptr) { - target = other_message.New(arena_); + target = other_message.New(arena); extension->repeated_message_value->AddAllocated(target); } target->CheckTypeAndMergeFrom(other_message); } break; + } } } else { if (!other_extension.is_cleared) { @@ -1041,6 +1047,7 @@ void ExtensionSet::InternalExtensionMergeFrom(const MessageLite* extendee, other_extension.descriptor); break; case WireFormatLite::CPPTYPE_MESSAGE: { + Arena* const arena = arena_; Extension* extension; bool is_new = MaybeNewExtension(number, other_extension.descriptor, &extension); @@ -1051,14 +1058,14 @@ void ExtensionSet::InternalExtensionMergeFrom(const MessageLite* extendee, if (other_extension.is_lazy) { extension->is_lazy = true; extension->lazymessage_value = - other_extension.lazymessage_value->New(arena_); + other_extension.lazymessage_value->New(arena); extension->lazymessage_value->MergeFrom( GetPrototypeForLazyMessage(extendee, number), - *other_extension.lazymessage_value, arena_); + *other_extension.lazymessage_value, arena); } else { extension->is_lazy = false; extension->message_value = - other_extension.message_value->New(arena_); + other_extension.message_value->New(arena); extension->message_value->CheckTypeAndMergeFrom( *other_extension.message_value); } @@ -1070,7 +1077,7 @@ void ExtensionSet::InternalExtensionMergeFrom(const MessageLite* extendee, if (extension->is_lazy) { extension->lazymessage_value->MergeFrom( GetPrototypeForLazyMessage(extendee, number), - *other_extension.lazymessage_value, arena_); + *other_extension.lazymessage_value, arena); } else { extension->message_value->CheckTypeAndMergeFrom( other_extension.lazymessage_value->GetMessage( @@ -1079,7 +1086,7 @@ void ExtensionSet::InternalExtensionMergeFrom(const MessageLite* extendee, } else { if (extension->is_lazy) { extension->lazymessage_value - ->MutableMessage(*other_extension.message_value, arena_) + ->MutableMessage(*other_extension.message_value, arena) ->CheckTypeAndMergeFrom(*other_extension.message_value); } else { extension->message_value->CheckTypeAndMergeFrom( @@ -1097,9 +1104,9 @@ void ExtensionSet::InternalExtensionMergeFrom(const MessageLite* extendee, void ExtensionSet::Swap(const MessageLite* extendee, ExtensionSet* other) { #ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() != nullptr && GetArena() == other->GetArena()) { + if (arena_ != nullptr && arena_ == other->arena_) { #else // PROTOBUF_FORCE_COPY_IN_SWAP - if (GetArena() == other->GetArena()) { + if (arena_ == other->arena_) { #endif // !PROTOBUF_FORCE_COPY_IN_SWAP InternalSwap(other); } else { @@ -1127,7 +1134,9 @@ void ExtensionSet::SwapExtension(const MessageLite* extendee, ExtensionSet* other, int number) { if (this == other) return; - if (GetArena() == other->GetArena()) { + Arena* const arena = arena_; + Arena* const other_arena = other->arena_; + if (arena == other_arena) { UnsafeShallowSwapExtension(other, number); return; } @@ -1144,23 +1153,20 @@ void ExtensionSet::SwapExtension(const MessageLite* extendee, // We do it this way to reuse the copy-across-arenas logic already // implemented in ExtensionSet's MergeFrom. ExtensionSet temp; - temp.InternalExtensionMergeFrom(extendee, number, *other_ext, - other->GetArena()); + temp.InternalExtensionMergeFrom(extendee, number, *other_ext, other_arena); Extension* temp_ext = temp.FindOrNull(number); other_ext->Clear(); - other->InternalExtensionMergeFrom(extendee, number, *this_ext, - this->GetArena()); + other->InternalExtensionMergeFrom(extendee, number, *this_ext, arena); this_ext->Clear(); InternalExtensionMergeFrom(extendee, number, *temp_ext, temp.GetArena()); } else if (this_ext == nullptr) { - InternalExtensionMergeFrom(extendee, number, *other_ext, other->GetArena()); - if (other->GetArena() == nullptr) other_ext->Free(); + InternalExtensionMergeFrom(extendee, number, *other_ext, other_arena); + if (other_arena == nullptr) other_ext->Free(); other->Erase(number); } else { - other->InternalExtensionMergeFrom(extendee, number, *this_ext, - this->GetArena()); - if (GetArena() == nullptr) this_ext->Free(); + other->InternalExtensionMergeFrom(extendee, number, *this_ext, arena); + if (arena == nullptr) this_ext->Free(); Erase(number); } } @@ -1173,7 +1179,7 @@ void ExtensionSet::UnsafeShallowSwapExtension(ExtensionSet* other, int number) { if (this_ext == other_ext) return; - ABSL_DCHECK_EQ(GetArena(), other->GetArena()); + ABSL_DCHECK_EQ(arena_, other->arena_); if (this_ext != nullptr && other_ext != nullptr) { std::swap(*this_ext, *other_ext); @@ -1189,16 +1195,17 @@ void ExtensionSet::UnsafeShallowSwapExtension(ExtensionSet* other, int number) { bool ExtensionSet::IsInitialized(const MessageLite* extendee) const { // Extensions are never required. However, we need to check that all // embedded messages are initialized. + Arena* const arena = arena_; if (PROTOBUF_PREDICT_FALSE(is_large())) { for (const auto& kv : *map_.large) { - if (!kv.second.IsInitialized(this, extendee, kv.first, arena_)) { + if (!kv.second.IsInitialized(this, extendee, kv.first, arena)) { return false; } } return true; } for (const KeyValue* it = flat_begin(); it != flat_end(); ++it) { - if (!it->second.IsInitialized(this, extendee, it->first, arena_)) { + if (!it->second.IsInitialized(this, extendee, it->first, arena)) { return false; } } @@ -1639,8 +1646,9 @@ void ExtensionSet::GrowCapacity(size_t minimum_new_capacity) { const KeyValue* begin = flat_begin(); const KeyValue* end = flat_end(); AllocatedData new_map; + Arena* const arena = arena_; if (new_flat_capacity > kMaximumFlatCapacity) { - new_map.large = Arena::Create(arena_); + new_map.large = Arena::Create(arena); LargeMap::iterator hint = new_map.large->begin(); for (const KeyValue* it = begin; it != end; ++it) { hint = new_map.large->insert(hint, {it->first, it->second}); @@ -1648,11 +1656,11 @@ void ExtensionSet::GrowCapacity(size_t minimum_new_capacity) { flat_size_ = static_cast(-1); ABSL_DCHECK(is_large()); } else { - new_map.flat = Arena::CreateArray(arena_, new_flat_capacity); + new_map.flat = Arena::CreateArray(arena, new_flat_capacity); std::copy(begin, end, new_map.flat); } - if (arena_ == nullptr) { + if (arena == nullptr) { DeleteFlatMap(begin, flat_capacity_); } flat_capacity_ = new_flat_capacity; diff --git a/src/google/protobuf/generated_message_reflection.cc b/src/google/protobuf/generated_message_reflection.cc index eb60d5fa32..c75c29004c 100644 --- a/src/google/protobuf/generated_message_reflection.cc +++ b/src/google/protobuf/generated_message_reflection.cc @@ -1044,21 +1044,24 @@ void Reflection::SwapOneofField(Message* lhs, Message* rhs, } } -void Reflection::Swap(Message* message1, Message* message2) const { - if (message1 == message2) return; +void Reflection::Swap(Message* lhs, Message* rhs) const { + if (lhs == rhs) return; + + Arena* lhs_arena = lhs->GetArena(); + Arena* rhs_arena = rhs->GetArena(); // TODO: Other Reflection methods should probably check this too. - ABSL_CHECK_EQ(message1->GetReflection(), this) + ABSL_CHECK_EQ(lhs->GetReflection(), this) << "First argument to Swap() (of type \"" - << message1->GetDescriptor()->full_name() + << lhs->GetDescriptor()->full_name() << "\") is not compatible with this reflection object (which is for type " "\"" << descriptor_->full_name() << "\"). Note that the exact same class is required; not just the same " "descriptor."; - ABSL_CHECK_EQ(message2->GetReflection(), this) + ABSL_CHECK_EQ(rhs->GetReflection(), this) << "Second argument to Swap() (of type \"" - << message2->GetDescriptor()->full_name() + << rhs->GetDescriptor()->full_name() << "\") is not compatible with this reflection object (which is for type " "\"" << descriptor_->full_name() @@ -1068,32 +1071,31 @@ void Reflection::Swap(Message* message1, Message* message2) const { // Check that both messages are in the same arena (or both on the heap). We // need to copy all data if not, due to ownership semantics. #ifdef PROTOBUF_FORCE_COPY_IN_SWAP - if (message1->GetArena() == nullptr || - message1->GetArena() != message2->GetArena()) { + if (lhs_arena == nullptr || lhs_arena != rhs_arena) { #else // PROTOBUF_FORCE_COPY_IN_SWAP - if (message1->GetArena() != message2->GetArena()) { + if (lhs_arena != rhs_arena) { #endif // !PROTOBUF_FORCE_COPY_IN_SWAP // One of the two is guaranteed to have an arena. Switch things around - // to guarantee that message1 has an arena. - Arena* arena = message1->GetArena(); + // to guarantee that lhs has an arena. + Arena* arena = lhs_arena; if (arena == nullptr) { - arena = message2->GetArena(); - std::swap(message1, message2); // Swapping names for pointers! + arena = rhs_arena; + std::swap(lhs, rhs); // Swapping names for pointers! } - Message* temp = message1->New(arena); - temp->MergeFrom(*message2); - message2->CopyFrom(*message1); + Message* temp = lhs->New(arena); + temp->MergeFrom(*rhs); + rhs->CopyFrom(*lhs); #ifdef PROTOBUF_FORCE_COPY_IN_SWAP - message1->CopyFrom(*temp); + lhs->CopyFrom(*temp); if (arena == nullptr) delete temp; #else // PROTOBUF_FORCE_COPY_IN_SWAP - Swap(message1, temp); + Swap(lhs, temp); #endif // !PROTOBUF_FORCE_COPY_IN_SWAP return; } - UnsafeArenaSwap(message1, message2); + UnsafeArenaSwap(lhs, rhs); } template @@ -2318,27 +2320,34 @@ void Reflection::SetAllocatedMessage(Message* message, Message* sub_message, ABSL_DCHECK(sub_message == nullptr || sub_message->GetArena() == nullptr || sub_message->GetArena() == message->GetArena()); + if (sub_message == nullptr) { + UnsafeArenaSetAllocatedMessage(message, nullptr, field); + return; + } + + Arena* arena = message->GetArena(); + Arena* sub_arena = sub_message->GetArena(); + if (arena == sub_arena) { + UnsafeArenaSetAllocatedMessage(message, sub_message, field); + return; + } + // If message and sub-message are in different memory ownership domains // (different arenas, or one is on heap and one is not), then we may need to // do a copy. - if (sub_message != nullptr && - sub_message->GetArena() != message->GetArena()) { - if (sub_message->GetArena() == nullptr && message->GetArena() != nullptr) { - // Case 1: parent is on an arena and child is heap-allocated. We can add - // the child to the arena's Own() list to free on arena destruction, then - // set our pointer. - message->GetArena()->Own(sub_message); - UnsafeArenaSetAllocatedMessage(message, sub_message, field); - } else { - // Case 2: all other cases. We need to make a copy. MutableMessage() will - // either get the existing message object, or instantiate a new one as - // appropriate w.r.t. our arena. - Message* sub_message_copy = MutableMessage(message, field); - sub_message_copy->CopyFrom(*sub_message); - } - } else { - // Same memory ownership domains. + if (sub_arena == nullptr) { + ABSL_DCHECK_NE(arena, nullptr); + // Case 1: parent is on an arena and child is heap-allocated. We can add + // the child to the arena's Own() list to free on arena destruction, then + // set our pointer. + arena->Own(sub_message); UnsafeArenaSetAllocatedMessage(message, sub_message, field); + } else { + // Case 2: all other cases. We need to make a copy. MutableMessage() will + // either get the existing message object, or instantiate a new one as + // appropriate w.r.t. our arena. + Message* sub_message_copy = MutableMessage(message, field); + sub_message_copy->CopyFrom(*sub_message); } } diff --git a/src/google/protobuf/repeated_ptr_field.cc b/src/google/protobuf/repeated_ptr_field.cc index 2afe3adf7d..929131ce57 100644 --- a/src/google/protobuf/repeated_ptr_field.cc +++ b/src/google/protobuf/repeated_ptr_field.cc @@ -97,10 +97,11 @@ void RepeatedPtrFieldBase::DestroyProtos() { template auto* RepeatedPtrFieldBase::AddInternal(F factory) { - using Result = decltype(factory(GetArena())); + Arena* const arena = GetArena(); + using Result = decltype(factory(arena)); if (tagged_rep_or_elem_ == nullptr) { ExchangeCurrentSize(1); - tagged_rep_or_elem_ = factory(GetArena()); + tagged_rep_or_elem_ = factory(arena); return static_cast(tagged_rep_or_elem_); } if (using_sso()) { @@ -122,7 +123,7 @@ auto* RepeatedPtrFieldBase::AddInternal(F factory) { Rep* r = rep(); ++r->allocated_size; void*& result = r->elements[ExchangeCurrentSize(current_size_ + 1)]; - result = factory(GetArena()); + result = factory(arena); return static_cast(result); }