From 7013de632d289f660feb7a75a6d7fcef3aa0e8f4 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 19 Dec 2024 14:20:31 -0800 Subject: [PATCH] Clean up the implementation of InternalSwap in MapField: - Remove "UnsafeShallowSwap". The operation is called "InternalSwap" in all other types. - Remove the dynamic dispatch. It is no longer needed. - Make "InternalSwap" do the whole swap, including the map and the reflection payload. - Rename MapFieldBase::SwapImpl to SwapPayload to make it clearer on what it does and to prevent using it directly in the vtable. PiperOrigin-RevId: 708031253 --- src/google/protobuf/dynamic_message.cc | 7 +------ .../protobuf/generated_message_reflection.cc | 2 +- src/google/protobuf/map_field.cc | 13 +++++-------- src/google/protobuf/map_field.h | 14 ++------------ src/google/protobuf/map_field_inl.h | 16 +--------------- 5 files changed, 10 insertions(+), 42 deletions(-) diff --git a/src/google/protobuf/dynamic_message.cc b/src/google/protobuf/dynamic_message.cc index 561047f2f00fa..7c1f6539ee9f6 100644 --- a/src/google/protobuf/dynamic_message.cc +++ b/src/google/protobuf/dynamic_message.cc @@ -217,11 +217,6 @@ class DynamicMapField final static bool LookupMapValueImpl(const MapFieldBase& self, const MapKey& map_key, MapValueConstRef* val); - static void UnsafeShallowSwapImpl(MapFieldBase& lhs, MapFieldBase& rhs) { - static_cast(lhs).Swap( - static_cast(&rhs)); - } - static size_t SpaceUsedExcludingSelfNoLockImpl(const MapFieldBase& map); static const Message* GetPrototypeImpl(const MapFieldBase& map); @@ -358,7 +353,7 @@ void DynamicMapField::SwapImpl(MapFieldBase& lhs_base, MapFieldBase& rhs_base) { rhs.Clear(); rhs.MergeFrom(tmp); - MapFieldBase::SwapImpl(lhs, rhs); + MapFieldBase::SwapPayload(lhs, rhs); } void DynamicMapField::MergeFromImpl(MapFieldBase& base, diff --git a/src/google/protobuf/generated_message_reflection.cc b/src/google/protobuf/generated_message_reflection.cc index d6a68da7fda48..f7d2d5a72a754 100644 --- a/src/google/protobuf/generated_message_reflection.cc +++ b/src/google/protobuf/generated_message_reflection.cc @@ -767,7 +767,7 @@ void SwapFieldHelper::SwapRepeatedMessageField(const Reflection* r, auto* lhs_map = r->MutableRaw(lhs, field); auto* rhs_map = r->MutableRaw(rhs, field); if (unsafe_shallow_swap) { - lhs_map->UnsafeShallowSwap(rhs_map); + lhs_map->InternalSwap(rhs_map); } else { lhs_map->Swap(rhs_map); } diff --git a/src/google/protobuf/map_field.cc b/src/google/protobuf/map_field.cc index 00cd7bc5ab057..2408cfd8412ba 100644 --- a/src/google/protobuf/map_field.cc +++ b/src/google/protobuf/map_field.cc @@ -167,9 +167,9 @@ MapFieldBase::ReflectionPayload& MapFieldBase::PayloadSlow() const { return *ToPayload(p); } -void MapFieldBase::SwapImpl(MapFieldBase& lhs, MapFieldBase& rhs) { +void MapFieldBase::SwapPayload(MapFieldBase& lhs, MapFieldBase& rhs) { if (lhs.arena() == rhs.arena()) { - lhs.InternalSwap(&rhs); + SwapRelaxed(lhs.payload_, rhs.payload_); return; } auto* p1 = lhs.maybe_payload(); @@ -182,13 +182,10 @@ void MapFieldBase::SwapImpl(MapFieldBase& lhs, MapFieldBase& rhs) { SwapRelaxed(p1->state, p2->state); } -void MapFieldBase::UnsafeShallowSwapImpl(MapFieldBase& lhs, MapFieldBase& rhs) { - ABSL_DCHECK_EQ(lhs.arena(), rhs.arena()); - lhs.InternalSwap(&rhs); -} - void MapFieldBase::InternalSwap(MapFieldBase* other) { - SwapRelaxed(payload_, other->payload_); + ABSL_DCHECK_EQ(arena(), other->arena()); + GetMapRaw().InternalSwap(&other->GetMapRaw()); + SwapPayload(*this, *other); } size_t MapFieldBase::SpaceUsedExcludingSelfLong() const { diff --git a/src/google/protobuf/map_field.h b/src/google/protobuf/map_field.h index 5b1874ee4d4aa..262c443440b39 100644 --- a/src/google/protobuf/map_field.h +++ b/src/google/protobuf/map_field.h @@ -312,7 +312,6 @@ class PROTOBUF_EXPORT MapFieldBase : public MapFieldBaseForParse { void (*clear_map_no_sync)(MapFieldBase& map); void (*merge_from)(MapFieldBase& map, const MapFieldBase& other); void (*swap)(MapFieldBase& lhs, MapFieldBase& rhs); - void (*unsafe_shallow_swap)(MapFieldBase& lhs, MapFieldBase& rhs); size_t (*space_used_excluding_self_nolock)(const MapFieldBase& map); const Message* (*get_prototype)(const MapFieldBase& map); @@ -328,7 +327,6 @@ class PROTOBUF_EXPORT MapFieldBase : public MapFieldBaseForParse { out.clear_map_no_sync = &T::ClearMapNoSyncImpl; out.merge_from = &T::MergeFromImpl; out.swap = &T::SwapImpl; - out.unsafe_shallow_swap = &T::UnsafeShallowSwapImpl; out.space_used_excluding_self_nolock = &T::SpaceUsedExcludingSelfNoLockImpl; out.get_prototype = &T::GetPrototypeImpl; return out; @@ -366,9 +364,7 @@ class PROTOBUF_EXPORT MapFieldBase : public MapFieldBaseForParse { vtable()->merge_from(*this, other); } void Swap(MapFieldBase* other) { vtable()->swap(*this, *other); } - void UnsafeShallowSwap(MapFieldBase* other) { - vtable()->unsafe_shallow_swap(*this, *other); - } + void InternalSwap(MapFieldBase* other); // Sync Map with repeated field and returns the size of map. int size() const; void Clear(); @@ -411,8 +407,7 @@ class PROTOBUF_EXPORT MapFieldBase : public MapFieldBaseForParse { void SyncMapWithRepeatedField() const; void SyncMapWithRepeatedFieldNoLock(); - static void SwapImpl(MapFieldBase& lhs, MapFieldBase& rhs); - static void UnsafeShallowSwapImpl(MapFieldBase& lhs, MapFieldBase& rhs); + static void SwapPayload(MapFieldBase& lhs, MapFieldBase& rhs); static size_t SpaceUsedExcludingSelfNoLockImpl(const MapFieldBase& map); // Tells MapFieldBase that there is new change to Map. @@ -436,8 +431,6 @@ class PROTOBUF_EXPORT MapFieldBase : public MapFieldBaseForParse { return vtable()->insert_or_lookup_no_sync(*this, map_key, val); } - void InternalSwap(MapFieldBase* other); - // Support thread sanitizer (tsan) by making const / mutable races // more apparent. If one thread calls MutableAccess() while another // thread calls either ConstAccess() or MutableAccess(), on the same @@ -601,8 +594,6 @@ class TypeDefinedMapFieldBase : public MapFieldBase { return &map_; } - void InternalSwap(TypeDefinedMapFieldBase* other); - static constexpr size_t InternalGetArenaOffsetAlt( internal::InternalVisibility access) { return PROTOBUF_FIELD_OFFSET(TypeDefinedMapFieldBase, map_) + @@ -621,7 +612,6 @@ class TypeDefinedMapFieldBase : public MapFieldBase { static void MergeFromImpl(MapFieldBase& base, const MapFieldBase& other); static void SwapImpl(MapFieldBase& lhs, MapFieldBase& rhs); - static void UnsafeShallowSwapImpl(MapFieldBase& lhs, MapFieldBase& rhs); // map_ is inside an anonymous union so we can explicitly control its // destruction diff --git a/src/google/protobuf/map_field_inl.h b/src/google/protobuf/map_field_inl.h index 45c782bb1ae90..c4bf91f9e8f6d 100644 --- a/src/google/protobuf/map_field_inl.h +++ b/src/google/protobuf/map_field_inl.h @@ -81,7 +81,7 @@ bool TypeDefinedMapFieldBase::DeleteMapValueImpl( template void TypeDefinedMapFieldBase::SwapImpl(MapFieldBase& lhs, MapFieldBase& rhs) { - MapFieldBase::SwapImpl(lhs, rhs); + MapFieldBase::SwapPayload(lhs, rhs); static_cast(lhs).map_.swap( static_cast(rhs).map_); } @@ -97,20 +97,6 @@ void TypeDefinedMapFieldBase::MergeFromImpl(MapFieldBase& base, self.SetMapDirty(); } -template -void TypeDefinedMapFieldBase::UnsafeShallowSwapImpl(MapFieldBase& lhs, - MapFieldBase& rhs) { - static_cast(lhs).InternalSwap( - static_cast(&rhs)); -} - -template -void TypeDefinedMapFieldBase::InternalSwap( - TypeDefinedMapFieldBase* other) { - MapFieldBase::InternalSwap(other); - map_.InternalSwap(&other->map_); -} - // ---------------------------------------------------------------------- template