diff --git a/src/google/protobuf/arena.cc b/src/google/protobuf/arena.cc index 99f247fd82..2a624145c5 100644 --- a/src/google/protobuf/arena.cc +++ b/src/google/protobuf/arena.cc @@ -603,7 +603,6 @@ void ThreadSafeArena::AddSerialArena(void* id, SerialArena* serial) { } void ThreadSafeArena::Init() { - // Message-owned arenas bypass thread cache and do not need life cycle ID. tag_and_id_ = GetNextLifeCycleId(); arena_stats_ = Sample(); head_.store(SentrySerialArenaChunk(), std::memory_order_relaxed); diff --git a/src/google/protobuf/compiler/cpp/message.cc b/src/google/protobuf/compiler/cpp/message.cc index 3ac44ad13c..0f293a90e0 100644 --- a/src/google/protobuf/compiler/cpp/message.cc +++ b/src/google/protobuf/compiler/cpp/message.cc @@ -3602,20 +3602,21 @@ void MessageGenerator::GenerateCopyFrom(io::Printer* p) { // It is also disabled if a message has neither message fields nor // extensions, as it's impossible to copy from its descendant. // - // Note that FailIfCopyFromDescendant is implemented by reflection and not - // available for lite runtime. In that case, check if the size of the source - // has changed after Clear. - format("#ifndef NDEBUG\n"); + // Note that IsDescendant is implemented by reflection and not available for + // lite runtime. In that case, check if the size of the source has changed + // after Clear. if (HasDescriptorMethods(descriptor_->file(), options_)) { - format("FailIfCopyFromDescendant(*this, from);\n"); + format( + "$DCHK$(!::_pbi::IsDescendant(*this, from))\n" + " << \"Source of CopyFrom cannot be a descendant of the " + "target.\";\n" + "Clear();\n"); } else { - format("::size_t from_size = from.ByteSizeLong();\n"); - } - format( - "#endif\n" - "Clear();\n"); - if (!HasDescriptorMethods(descriptor_->file(), options_)) { format( + "#ifndef NDEBUG\n" + "::size_t from_size = from.ByteSizeLong();\n" + "#endif\n" + "Clear();\n" "#ifndef NDEBUG\n" "$CHK$_EQ(from_size, from.ByteSizeLong())\n" " << \"Source of CopyFrom changed when clearing target. Either \"\n" diff --git a/src/google/protobuf/extension_set_unittest.cc b/src/google/protobuf/extension_set_unittest.cc index 5a351405f4..94e4bbbc4b 100644 --- a/src/google/protobuf/extension_set_unittest.cc +++ b/src/google/protobuf/extension_set_unittest.cc @@ -829,8 +829,7 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { // Repeated primitive extensions will increase space used by at least a // RepeatedField, and will cause additional allocations when the array - // gets too big for the initial space. Note, we explicitly allocate on the - // heap to avoid message-owned arenas. + // gets too big for the initial space. // This macro: // - Adds a value to the repeated extension, then clears it, establishing // the base size. diff --git a/src/google/protobuf/generated_message_reflection_unittest.cc b/src/google/protobuf/generated_message_reflection_unittest.cc index 8c2b90a12a..d4a1e1337d 100644 --- a/src/google/protobuf/generated_message_reflection_unittest.cc +++ b/src/google/protobuf/generated_message_reflection_unittest.cc @@ -614,10 +614,16 @@ TEST(GeneratedMessageReflectionTest, ReleaseLast) { (void)expected; // unused in somce configurations std::unique_ptr released(message.GetReflection()->ReleaseLast( &message, descriptor->FindFieldByName("repeated_foreign_message"))); +#ifndef PROTOBUF_FORCE_COPY_IN_RELEASE EXPECT_EQ(expected, released.get()); +#endif // !PROTOBUF_FORCE_COPY_IN_RELEASE } TEST(GeneratedMessageReflectionTest, ReleaseLastExtensions) { +#ifdef PROTOBUF_FORCE_COPY_IN_RELEASE + GTEST_SKIP() << "Won't work with FORCE_COPY_IN_RELEASE."; +#endif // !PROTOBUF_FORCE_COPY_IN_RELEASE + unittest::TestAllExtensions message; const Descriptor* descriptor = message.GetDescriptor(); TestUtil::ReflectionTester reflection_tester(descriptor); @@ -1041,7 +1047,9 @@ TEST(GeneratedMessageReflectionTest, SetAllocatedOneofMessageTest) { released = reflection->ReleaseMessage( &to_message, descriptor->FindFieldByName("foo_lazy_message")); EXPECT_TRUE(released != nullptr); +#ifndef PROTOBUF_FORCE_COPY_IN_RELEASE EXPECT_EQ(&sub_message, released); +#endif // !PROTOBUF_FORCE_COPY_IN_RELEASE delete released; TestUtil::ReflectionTester::SetOneofViaReflection(&from_message2); @@ -1059,7 +1067,9 @@ TEST(GeneratedMessageReflectionTest, SetAllocatedOneofMessageTest) { released = reflection->ReleaseMessage( &to_message, descriptor->FindFieldByName("foo_message")); EXPECT_TRUE(released != nullptr); +#ifndef PROTOBUF_FORCE_COPY_IN_RELEASE EXPECT_EQ(&sub_message2, released); +#endif // !PROTOBUF_FORCE_COPY_IN_RELEASE delete released; } @@ -1180,7 +1190,9 @@ TEST(GeneratedMessageReflectionTest, ReleaseOneofMessageTest) { &message, descriptor->FindFieldByName("foo_lazy_message")); EXPECT_TRUE(released != nullptr); +#ifndef PROTOBUF_FORCE_COPY_IN_RELEASE EXPECT_EQ(&sub_message, released); +#endif // !PROTOBUF_FORCE_COPY_IN_RELEASE delete released; released = reflection->ReleaseMessage( diff --git a/src/google/protobuf/map_test.inc b/src/google/protobuf/map_test.inc index c57cf5c652..78a4808296 100644 --- a/src/google/protobuf/map_test.inc +++ b/src/google/protobuf/map_test.inc @@ -3170,7 +3170,9 @@ TEST(GeneratedMapFieldReflectionTest, ReleaseLast) { std::vector release_last = MapTestUtil::GetMapEntriesFromRelease(&message); MapTestUtil::ExpectMapsSize(message, 1); +#ifndef PROTOBUF_FORCE_COPY_IN_RELEASE EXPECT_TRUE(expect_last == release_last); +#endif // !PROTOBUF_FORCE_COPY_IN_RELEASE for (std::vector::iterator it = release_last.begin(); it != release_last.end(); ++it) { delete *it; diff --git a/src/google/protobuf/message.cc b/src/google/protobuf/message.cc index b2ae719b9f..7191ba5fd6 100644 --- a/src/google/protobuf/message.cc +++ b/src/google/protobuf/message.cc @@ -121,22 +121,14 @@ void Message::CopyFrom(const Message& from) { } void Message::CopyWithSourceCheck(Message& to, const Message& from) { -#ifndef NDEBUG - FailIfCopyFromDescendant(to, from); -#endif + // Fail if "from" is a descendant of "to" as such copy is not allowed. + GOOGLE_ABSL_DCHECK(!internal::IsDescendant(to, from)) + << "Source of CopyFrom cannot be a descendant of the target."; + to.Clear(); to.GetClassData()->merge_to_from(to, from); } -void Message::FailIfCopyFromDescendant(Message& to, const Message& from) { - auto* arena = to.GetArenaForAllocation(); - bool same_message_owned_arena = to.GetOwningArena() == nullptr && - arena != nullptr && - arena == from.GetOwningArena(); - GOOGLE_ABSL_CHECK(!same_message_owned_arena && !internal::IsDescendant(to, from)) - << "Source of CopyFrom cannot be a descendant of the target."; -} - std::string Message::GetTypeName() const { return GetDescriptor()->full_name(); } diff --git a/src/google/protobuf/message.h b/src/google/protobuf/message.h index 0ede96e8c9..28b9fc70f8 100644 --- a/src/google/protobuf/message.h +++ b/src/google/protobuf/message.h @@ -412,9 +412,6 @@ class PROTOBUF_EXPORT Message : public MessageLite { // type, and thus uses GetClassData(). static void CopyWithSourceCheck(Message& to, const Message& from); - // Fail if "from" is a descendant of "to" as such copy is not allowed. - static void FailIfCopyFromDescendant(Message& to, const Message& from); - inline explicit Message(Arena* arena) : MessageLite(arena) {} size_t ComputeUnknownFieldsSize(size_t total_size, internal::CachedSize* cached_size) const; diff --git a/src/google/protobuf/message_lite.h b/src/google/protobuf/message_lite.h index 663321a8b4..7f266b3d64 100644 --- a/src/google/protobuf/message_lite.h +++ b/src/google/protobuf/message_lite.h @@ -187,8 +187,7 @@ class PROTOBUF_EXPORT MessageLite { // if arena is a nullptr. virtual MessageLite* New(Arena* arena) const = 0; - // Returns user-owned arena; nullptr if it's message owned. - Arena* GetArena() const { return _internal_metadata_.user_arena(); } + Arena* GetArena() const { return _internal_metadata_.arena(); } // Clear all fields of the message and set them to their default values. // Clear() assumes that any memory allocated to hold parts of the message @@ -436,7 +435,7 @@ class PROTOBUF_EXPORT MessageLite { // internal memory). This method is used in proto's implementation for // swapping, moving and setting allocated, for deciding whether the ownership // of this message or its internal memory could be changed. - Arena* GetOwningArena() const { return _internal_metadata_.owning_arena(); } + Arena* GetOwningArena() const { return _internal_metadata_.arena(); } // Returns the arena, used for allocating internal objects(e.g., child // messages, etc), or owning incoming objects (e.g., set allocated). diff --git a/src/google/protobuf/metadata_lite.h b/src/google/protobuf/metadata_lite.h index cb75e57e07..66c9440861 100644 --- a/src/google/protobuf/metadata_lite.h +++ b/src/google/protobuf/metadata_lite.h @@ -95,10 +95,6 @@ class PROTOBUF_EXPORT InternalMetadata { } } - PROTOBUF_NDEBUG_INLINE Arena* owning_arena() const { return arena(); } - - PROTOBUF_NDEBUG_INLINE Arena* user_arena() const { return arena(); } - PROTOBUF_NDEBUG_INLINE Arena* arena() const { if (PROTOBUF_PREDICT_FALSE(have_unknown_fields())) { return PtrValue()->arena; diff --git a/src/google/protobuf/port_def.inc b/src/google/protobuf/port_def.inc index fb7505c355..1a05c4fd1b 100644 --- a/src/google/protobuf/port_def.inc +++ b/src/google/protobuf/port_def.inc @@ -655,18 +655,6 @@ #error "endian detection failed for current compiler" #endif -// For enabling message owned arena, one major blocker is semantic change from -// moving to copying when there is ownership transfer (e.g., move ctor, swap, -// set allocated, release). This change not only causes performance regression -// but also breaks users code (e.g., dangling reference). For top-level -// messages, since it owns the arena, we can mitigate the issue by transferring -// ownership of arena. However, we cannot do that for nested messages. In order -// to tell how many usages of nested messages affected by message owned arena, -// we need to simulate the arena ownership. -// This experiment is purely for the purpose of gathering data. All code guarded -// by this flag is supposed to be removed after this experiment. -#define PROTOBUF_MESSAGE_OWNED_ARENA_EXPERIMENT - #ifdef PROTOBUF_CONSTINIT #error PROTOBUF_CONSTINIT was previously defined #endif diff --git a/src/google/protobuf/port_undef.inc b/src/google/protobuf/port_undef.inc index 217b49b642..c52eb30a4e 100644 --- a/src/google/protobuf/port_undef.inc +++ b/src/google/protobuf/port_undef.inc @@ -90,7 +90,6 @@ #undef PROTOBUF_THREAD_LOCAL #undef PROTOBUF_LITTLE_ENDIAN #undef PROTOBUF_BIG_ENDIAN -#undef PROTOBUF_MESSAGE_OWNED_ARENA_EXPERIMENT #undef PROTOBUF_CONSTINIT #undef PROTOBUF_CONSTEXPR #undef PROTOBUF_ATTRIBUTE_WEAK diff --git a/src/google/protobuf/repeated_ptr_field.h b/src/google/protobuf/repeated_ptr_field.h index 00b1e966d0..0d82a8e516 100644 --- a/src/google/protobuf/repeated_ptr_field.h +++ b/src/google/protobuf/repeated_ptr_field.h @@ -516,7 +516,6 @@ class PROTOBUF_EXPORT RepeatedPtrFieldBase { GOOGLE_ABSL_DCHECK(GetOwningArena() == nullptr) << "ReleaseCleared() can only be used on a RepeatedPtrField not on " << "an arena."; - GOOGLE_ABSL_DCHECK(GetOwningArena() == nullptr); GOOGLE_ABSL_DCHECK(rep_ != nullptr); GOOGLE_ABSL_DCHECK_GT(rep_->allocated_size, current_size_); return cast(rep_->elements[--rep_->allocated_size]);