Skip to content

Commit

Permalink
Clean up the implementation of InternalSwap in MapField:
Browse files Browse the repository at this point in the history
 - 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
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Dec 19, 2024
1 parent 7740bcb commit 7013de6
Show file tree
Hide file tree
Showing 5 changed files with 10 additions and 42 deletions.
7 changes: 1 addition & 6 deletions src/google/protobuf/dynamic_message.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<DynamicMapField&>(lhs).Swap(
static_cast<DynamicMapField*>(&rhs));
}

static size_t SpaceUsedExcludingSelfNoLockImpl(const MapFieldBase& map);

static const Message* GetPrototypeImpl(const MapFieldBase& map);
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/google/protobuf/generated_message_reflection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ void SwapFieldHelper::SwapRepeatedMessageField(const Reflection* r,
auto* lhs_map = r->MutableRaw<MapFieldBase>(lhs, field);
auto* rhs_map = r->MutableRaw<MapFieldBase>(rhs, field);
if (unsafe_shallow_swap) {
lhs_map->UnsafeShallowSwap(rhs_map);
lhs_map->InternalSwap(rhs_map);
} else {
lhs_map->Swap(rhs_map);
}
Expand Down
13 changes: 5 additions & 8 deletions src/google/protobuf/map_field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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 {
Expand Down
14 changes: 2 additions & 12 deletions src/google/protobuf/map_field.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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_) +
Expand All @@ -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
Expand Down
16 changes: 1 addition & 15 deletions src/google/protobuf/map_field_inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ bool TypeDefinedMapFieldBase<Key, T>::DeleteMapValueImpl(
template <typename Key, typename T>
void TypeDefinedMapFieldBase<Key, T>::SwapImpl(MapFieldBase& lhs,
MapFieldBase& rhs) {
MapFieldBase::SwapImpl(lhs, rhs);
MapFieldBase::SwapPayload(lhs, rhs);
static_cast<TypeDefinedMapFieldBase&>(lhs).map_.swap(
static_cast<TypeDefinedMapFieldBase&>(rhs).map_);
}
Expand All @@ -97,20 +97,6 @@ void TypeDefinedMapFieldBase<Key, T>::MergeFromImpl(MapFieldBase& base,
self.SetMapDirty();
}

template <typename Key, typename T>
void TypeDefinedMapFieldBase<Key, T>::UnsafeShallowSwapImpl(MapFieldBase& lhs,
MapFieldBase& rhs) {
static_cast<TypeDefinedMapFieldBase&>(lhs).InternalSwap(
static_cast<TypeDefinedMapFieldBase*>(&rhs));
}

template <typename Key, typename T>
void TypeDefinedMapFieldBase<Key, T>::InternalSwap(
TypeDefinedMapFieldBase* other) {
MapFieldBase::InternalSwap(other);
map_.InternalSwap(&other->map_);
}

// ----------------------------------------------------------------------

template <typename Derived, typename Key, typename T,
Expand Down

0 comments on commit 7013de6

Please sign in to comment.