Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up the implementation of InternalSwap in MapField: #19751

Merged
merged 1 commit into from
Dec 23, 2024
+9 −42
Merged

Clean up the implementation of InternalSwap in MapField: #19751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/google/protobuf/dynamic_message.cc
View file
Open in desktop
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
View file
Open in desktop
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
12 changes: 4 additions & 8 deletions src/google/protobuf/map_field.cc
View file
Open in desktop
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,9 @@ 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_);
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
View file
Open in desktop
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 @@ -605,8 +598,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 @@ -625,7 +616,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
View file
Open in desktop
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
Loading