Skip to content

Commit

Permalink
Use GetArena() instead of GetOwningArena() #3.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 573556031
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Oct 15, 2023
1 parent 4a24a29 commit 3bd0f35
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 68 deletions.
2 changes: 1 addition & 1 deletion src/google/protobuf/generated_message_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ void GenericSwap(MessageLite* m1, MessageLite* m2) {
MessageLite* GetOwnedMessageInternal(Arena* message_arena,
MessageLite* submessage,
Arena* submessage_arena) {
ABSL_DCHECK(Arena::InternalGetOwningArena(submessage) == submessage_arena);
ABSL_DCHECK(submessage->GetArena() == submessage_arena);
ABSL_DCHECK(message_arena != submessage_arena);
ABSL_DCHECK_EQ(submessage_arena, nullptr);
if (message_arena != nullptr && submessage_arena == nullptr) {
Expand Down
4 changes: 2 additions & 2 deletions src/google/protobuf/message.cc
Original file line number Diff line number Diff line change
Expand Up @@ -414,8 +414,8 @@ template <>
PROTOBUF_NOINLINE
#endif
Arena*
GenericTypeHandler<Message>::GetOwningArena(Message* value) {
return value->GetOwningArena();
GenericTypeHandler<Message>::GetArena(Message* value) {
return value->GetArena();
}

template void InternalMetadata::DoClear<UnknownFieldSet>();
Expand Down
13 changes: 0 additions & 13 deletions src/google/protobuf/proto3_arena_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,6 @@ using proto3_arena_unittest::TestAllTypes;

namespace google {
namespace protobuf {

namespace internal {

class Proto3ArenaTestHelper {
public:
template <typename T>
static Arena* GetOwningArena(const T& msg) {
return msg.GetOwningArena();
}
};

} // namespace internal

namespace {
// We selectively set/check a few representative fields rather than all fields
// as this test is only expected to cover the basics of arena support.
Expand Down
10 changes: 4 additions & 6 deletions src/google/protobuf/reflection_ops.cc
Original file line number Diff line number Diff line change
Expand Up @@ -415,16 +415,14 @@ void ReflectionOps::FindInitializationErrors(const Message& message,

void GenericSwap(Message* lhs, Message* rhs) {
#ifndef PROTOBUF_FORCE_COPY_IN_SWAP
ABSL_DCHECK(Arena::InternalGetOwningArena(lhs) !=
Arena::InternalGetOwningArena(rhs));
ABSL_DCHECK(Arena::InternalGetOwningArena(lhs) != nullptr ||
Arena::InternalGetOwningArena(rhs) != nullptr);
ABSL_DCHECK(lhs->GetArena() != rhs->GetArena());
ABSL_DCHECK(lhs->GetArena() != nullptr || rhs->GetArena() != nullptr);
#endif // !PROTOBUF_FORCE_COPY_IN_SWAP
// At least one of these must have an arena, so make `rhs` point to it.
Arena* arena = Arena::InternalGetOwningArena(rhs);
Arena* arena = rhs->GetArena();
if (arena == nullptr) {
std::swap(lhs, rhs);
arena = Arena::InternalGetOwningArena(rhs);
arena = rhs->GetArena();
}

// Improve efficiency by placing the temporary on an arena so that messages
Expand Down
19 changes: 9 additions & 10 deletions src/google/protobuf/repeated_field.h
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ RepeatedField<Element>::~RepeatedField() {
#ifndef NDEBUG
// Try to trigger segfault / asan failure in non-opt builds if arena_
// lifetime has ended before the destructor.
auto arena = GetOwningArena();
auto arena = GetArena();
if (arena) (void)arena->SpaceAllocated();
#endif
if (total_size_ > 0) {
Expand All @@ -526,7 +526,7 @@ inline RepeatedField<Element>::RepeatedField(RepeatedField&& other) noexcept
// We don't just call Swap(&other) here because it would perform 3 copies if
// other is on an arena. This field can't be on an arena because arena
// construction always uses the Arena* accepting constructor.
if (other.GetOwningArena()) {
if (other.GetArena()) {
CopyFrom(other);
} else {
InternalSwap(&other);
Expand All @@ -540,9 +540,9 @@ inline RepeatedField<Element>& RepeatedField<Element>::operator=(
// We don't just call Swap(&other) here because it would perform 3 copies if
// the two fields are on different arenas.
if (this != &other) {
if (GetOwningArena() != other.GetOwningArena()
if (GetArena() != other.GetArena()
#ifdef PROTOBUF_FORCE_COPY_IN_MOVE
|| GetOwningArena() == nullptr
|| GetArena() == nullptr
#endif // !PROTOBUF_FORCE_COPY_IN_MOVE
) {
CopyFrom(other);
Expand Down Expand Up @@ -835,14 +835,13 @@ template <typename Element>
void RepeatedField<Element>::Swap(RepeatedField* other) {
if (this == other) return;
#ifdef PROTOBUF_FORCE_COPY_IN_SWAP
if (GetOwningArena() != nullptr &&
GetOwningArena() == other->GetOwningArena()) {
if (GetArena() != nullptr && GetArena() == other->GetArena()) {
#else // PROTOBUF_FORCE_COPY_IN_SWAP
if (GetOwningArena() == other->GetOwningArena()) {
if (GetArena() == other->GetArena()) {
#endif // !PROTOBUF_FORCE_COPY_IN_SWAP
InternalSwap(other);
} else {
RepeatedField<Element> temp(other->GetOwningArena());
RepeatedField<Element> temp(other->GetArena());
temp.MergeFrom(*this);
CopyFrom(*other);
other->UnsafeArenaSwap(&temp);
Expand All @@ -852,7 +851,7 @@ void RepeatedField<Element>::Swap(RepeatedField* other) {
template <typename Element>
void RepeatedField<Element>::UnsafeArenaSwap(RepeatedField* other) {
if (this == other) return;
ABSL_DCHECK_EQ(GetOwningArena(), other->GetOwningArena());
ABSL_DCHECK_EQ(GetArena(), other->GetArena());
InternalSwap(other);
}

Expand Down Expand Up @@ -941,7 +940,7 @@ PROTOBUF_NOINLINE void RepeatedField<Element>::GrowNoAnnotate(int current_size,
int new_size) {
ABSL_DCHECK_GT(new_size, total_size_);
Rep* new_rep;
Arena* arena = GetOwningArena();
Arena* arena = GetArena();

new_size = internal::CalculateReserveSize<Element, kRepHeaderSize>(
total_size_, new_size);
Expand Down
6 changes: 3 additions & 3 deletions src/google/protobuf/repeated_ptr_field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ void** RepeatedPtrFieldBase::InternalExtend(int extend_amount) {
ABSL_DCHECK(extend_amount > 0);
constexpr size_t ptr_size = sizeof(rep()->elements[0]);
int new_capacity = total_size_ + extend_amount;
Arena* arena = GetOwningArena();
Arena* arena = GetArena();
new_capacity = internal::CalculateReserveSize<void*, kRepHeaderSize>(
total_size_, new_capacity);
ABSL_CHECK_LE(
Expand Down Expand Up @@ -234,7 +234,7 @@ void RepeatedPtrFieldBase::MergeFromConcreteMessage(
dst += recycled;
src += recycled;
}
Arena* arena = GetOwningArena();
Arena* arena = GetArena();
for (; src < end; ++src, ++dst) {
*dst = copy_fn(arena, **src);
}
Expand All @@ -257,7 +257,7 @@ void RepeatedPtrFieldBase::MergeFrom<MessageLite>(
dst += recycled;
src += recycled;
}
Arena* arena = GetOwningArena();
Arena* arena = GetArena();
for (; src < end; ++src, ++dst) {
*dst = (*src)->New(arena);
(*dst)->CheckTypeAndMergeFrom(**src);
Expand Down
61 changes: 28 additions & 33 deletions src/google/protobuf/repeated_ptr_field.h
Original file line number Diff line number Diff line change
Expand Up @@ -398,10 +398,9 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
template <typename TypeHandler>
PROTOBUF_NDEBUG_INLINE void Swap(RepeatedPtrFieldBase* other) {
#ifdef PROTOBUF_FORCE_COPY_IN_SWAP
if (GetOwningArena() != nullptr &&
GetOwningArena() == other->GetOwningArena())
if (GetArena() != nullptr && GetArena() == other->GetArena())
#else // PROTOBUF_FORCE_COPY_IN_SWAP
if (GetOwningArena() == other->GetOwningArena())
if (GetArena() == other->GetArena())
#endif // !PROTOBUF_FORCE_COPY_IN_SWAP
{
InternalSwap(other);
Expand Down Expand Up @@ -507,10 +506,9 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {

template <typename TypeHandler>
void AddCleared(Value<TypeHandler>* value) {
ABSL_DCHECK(GetOwningArena() == nullptr)
<< "AddCleared() can only be used on a "
"RepeatedPtrField not on an arena.";
ABSL_DCHECK(TypeHandler::GetOwningArena(value) == nullptr)
ABSL_DCHECK(GetArena() == nullptr) << "AddCleared() can only be used on a "
"RepeatedPtrField not on an arena.";
ABSL_DCHECK(TypeHandler::GetArena(value) == nullptr)
<< "AddCleared() can only accept values not on an arena.";
MaybeExtend();
if (using_sso()) {
Expand All @@ -522,7 +520,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {

template <typename TypeHandler>
PROTOBUF_NODISCARD Value<TypeHandler>* ReleaseCleared() {
ABSL_DCHECK(GetOwningArena() == nullptr)
ABSL_DCHECK(GetArena() == nullptr)
<< "ReleaseCleared() can only be used on a RepeatedPtrField not on "
<< "an arena.";
ABSL_DCHECK(tagged_rep_or_elem_ != nullptr);
Expand All @@ -539,8 +537,8 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
// AddAllocated version that implements arena-safe copying behavior.
template <typename TypeHandler>
void AddAllocatedInternal(Value<TypeHandler>* value, std::true_type) {
Arena* element_arena = TypeHandler::GetOwningArena(value);
Arena* arena = GetOwningArena();
Arena* element_arena = TypeHandler::GetArena(value);
Arena* arena = GetArena();
if (arena == element_arena && allocated_size() < total_size_) {
// Fast path: underlying arena representation (tagged pointer) is equal to
// our arena pointer, and we can add to array without resizing it (at
Expand Down Expand Up @@ -606,7 +604,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
// First, release an element.
Value<TypeHandler>* result = UnsafeArenaReleaseLast<TypeHandler>();
// Now perform a copy if we're on an arena.
Arena* arena = GetOwningArena();
Arena* arena = GetArena();

#ifdef PROTOBUF_FORCE_COPY_IN_RELEASE
auto* new_result = copy<TypeHandler>(result);
Expand All @@ -623,7 +621,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
// this is the same as UnsafeArenaReleaseLast(). Note that we
// ABSL_DCHECK-fail if we're on an arena, since the user really should
// implement the copy operation in this case.
ABSL_DCHECK(GetOwningArena() == nullptr)
ABSL_DCHECK(GetArena() == nullptr)
<< "ReleaseLast() called on a RepeatedPtrField that is on an arena, "
<< "with a type that does not implement MergeFrom. This is unsafe; "
<< "please implement MergeFrom for your type.";
Expand All @@ -633,16 +631,15 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
template <typename TypeHandler>
PROTOBUF_NOINLINE void SwapFallback(RepeatedPtrFieldBase* other) {
#ifdef PROTOBUF_FORCE_COPY_IN_SWAP
ABSL_DCHECK(GetOwningArena() == nullptr ||
other->GetOwningArena() != GetOwningArena());
ABSL_DCHECK(GetArena() == nullptr || other->GetArena() != GetArena());
#else // PROTOBUF_FORCE_COPY_IN_SWAP
ABSL_DCHECK(other->GetOwningArena() != GetOwningArena());
ABSL_DCHECK(other->GetArena() != GetArena());
#endif // !PROTOBUF_FORCE_COPY_IN_SWAP

// Copy semantics in this case. We try to improve efficiency by placing the
// temporary on |other|'s arena so that messages are copied twice rather
// than three times.
RepeatedPtrFieldBase temp(other->GetOwningArena());
RepeatedPtrFieldBase temp(other->GetArena());
if (!this->empty()) {
temp.MergeFrom<typename TypeHandler::Type>(*this);
}
Expand All @@ -652,7 +649,7 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase {
}

// Gets the Arena on which this RepeatedPtrField stores its elements.
inline Arena* GetArena() const { return GetOwningArena(); }
inline Arena* GetArena() const { return arena_; }

inline Arena* GetOwningArena() const { return arena_; }

Expand Down Expand Up @@ -898,8 +895,8 @@ class GenericTypeHandler {
delete value;
#endif
}
static inline Arena* GetOwningArena(GenericType* value) {
return Arena::InternalGetOwningArena(value);
static inline Arena* GetArena(GenericType* value) {
return Arena::InternalGetArena(value);
}

static inline void Clear(GenericType* value) { value->Clear(); }
Expand All @@ -921,9 +918,8 @@ inline MessageLite* GenericTypeHandler<MessageLite>::NewFromPrototype(
return NewFromPrototypeHelper(prototype, arena);
}
template <>
inline Arena* GenericTypeHandler<MessageLite>::GetOwningArena(
MessageLite* value) {
return value->GetOwningArena();
inline Arena* GenericTypeHandler<MessageLite>::GetArena(MessageLite* value) {
return value->GetArena();
}

template <typename GenericType>
Expand All @@ -950,8 +946,7 @@ template <>
PROTOBUF_EXPORT Message* GenericTypeHandler<Message>::NewFromPrototype(
const Message* prototype, Arena* arena);
template <>
PROTOBUF_EXPORT Arena* GenericTypeHandler<Message>::GetOwningArena(
Message* value);
PROTOBUF_EXPORT Arena* GenericTypeHandler<Message>::GetArena(Message* value);

class StringTypeHandler {
public:
Expand All @@ -968,7 +963,7 @@ class StringTypeHandler {
Arena* arena) {
return New(arena);
}
static inline Arena* GetOwningArena(std::string*) { return nullptr; }
static inline Arena* GetArena(std::string*) { return nullptr; }
static inline void Delete(std::string* value, Arena* arena) {
if (arena == nullptr) {
delete value;
Expand Down Expand Up @@ -1408,7 +1403,7 @@ inline RepeatedPtrField<Element>::RepeatedPtrField(
// We don't just call Swap(&other) here because it would perform 3 copies if
// other is on an arena. This field can't be on an arena because arena
// construction always uses the Arena* accepting constructor.
if (other.GetOwningArena()) {
if (other.GetArena()) {
CopyFrom(other);
} else {
InternalSwap(&other);
Expand All @@ -1422,9 +1417,9 @@ inline RepeatedPtrField<Element>& RepeatedPtrField<Element>::operator=(
// We don't just call Swap(&other) here because it would perform 3 copies if
// the two fields are on different arenas.
if (this != &other) {
if (GetOwningArena() != other.GetOwningArena()
if (GetArena() != other.GetArena()
#ifdef PROTOBUF_FORCE_COPY_IN_MOVE
|| GetOwningArena() == nullptr
|| GetArena() == nullptr
#endif // !PROTOBUF_FORCE_COPY_IN_MOVE
) {
CopyFrom(other);
Expand Down Expand Up @@ -1506,7 +1501,7 @@ inline void RepeatedPtrField<Element>::DeleteSubrange(int start, int num) {
ABSL_DCHECK_GE(num, 0);
ABSL_DCHECK_LE(start + num, size());
void** subrange = raw_mutable_data() + start;
Arena* arena = GetOwningArena();
Arena* arena = GetArena();
for (int i = 0; i < num; ++i) {
using H = CommonHandler<TypeHandler>;
H::Delete(static_cast<Element*>(subrange[i]), arena);
Expand Down Expand Up @@ -1537,7 +1532,7 @@ inline void RepeatedPtrField<Element>::ExtractSubrangeInternal(
<< "Releasing elements without transferring ownership is an unsafe "
"operation. Use UnsafeArenaExtractSubrange.";
if (elements != nullptr) {
Arena* arena = GetOwningArena();
Arena* arena = GetArena();
auto* extracted = data() + start;
#ifdef PROTOBUF_FORCE_COPY_IN_RELEASE
// Always copy.
Expand Down Expand Up @@ -1573,7 +1568,7 @@ inline void RepeatedPtrField<Element>::ExtractSubrangeInternal(
// ExtractSubrange() must return heap-allocated objects by contract, and we
// cannot fulfill this contract if we are an on arena, we must ABSL_DCHECK()
// that we are not on an arena.
ABSL_DCHECK(GetOwningArena() == nullptr)
ABSL_DCHECK(GetArena() == nullptr)
<< "ExtractSubrange() when arena is non-nullptr is only supported when "
<< "the Element type supplies a MergeFrom() operation to make copies.";
UnsafeArenaExtractSubrange(start, num, elements);
Expand Down Expand Up @@ -1658,7 +1653,7 @@ template <typename Element>
inline void RepeatedPtrField<Element>::UnsafeArenaSwap(
RepeatedPtrField* other) {
if (this == other) return;
ABSL_DCHECK_EQ(GetOwningArena(), other->GetOwningArena());
ABSL_DCHECK_EQ(GetArena(), other->GetArena());
RepeatedPtrFieldBase::InternalSwap(other);
}

Expand All @@ -1681,7 +1676,7 @@ inline Arena* RepeatedPtrField<Element>::GetArena() const {

template <typename Element>
inline Arena* RepeatedPtrField<Element>::GetOwningArena() const {
return RepeatedPtrFieldBase::GetOwningArena();
return RepeatedPtrFieldBase::GetArena();
}

template <typename Element>
Expand Down

0 comments on commit 3bd0f35

Please sign in to comment.