From 578e07e9bd19b4ae959c57bbe4c1524d5642b58e Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Mon, 27 Nov 2023 11:26:16 -0800 Subject: [PATCH] Have Arena::Create support arena constructible types Unlike Arena::CreateMessage, Arena::Create creates only the top level object from arena even if it is arena constructalble; e.g. messages, RepeatedPtrField, etc. This renders arenas less effective. Instead of asking users to be aware of such nuances to use the right API for the right type, this CL makes Arena::Create recognizes and fully supports arena constructable types. While extremly rare, some users try to emulate Arena::CreateMessage with Arena::Create by passing arena parameter twice. For example, ``` auto foo = Arena::Create(&arena, &arena); // bad ``` This pattern is not supported and will break after this change. The following is recommended instead. ``` auto foo = Arena::CreateMessage(&arena); // recommended auto foo = Arena::Create(&arena); // after this change ``` PiperOrigin-RevId: 585709990 --- cmake/abseil-cpp.cmake | 1 + src/google/protobuf/BUILD.bazel | 1 + src/google/protobuf/arena.h | 51 ++++++++++++-------- src/google/protobuf/arena_unittest.cc | 23 +++++++++ src/google/protobuf/proto3_arena_unittest.cc | 2 +- 5 files changed, 58 insertions(+), 20 deletions(-) diff --git a/cmake/abseil-cpp.cmake b/cmake/abseil-cpp.cmake index b50fb89e6c3ed..a4e9d2295e3df 100644 --- a/cmake/abseil-cpp.cmake +++ b/cmake/abseil-cpp.cmake @@ -72,6 +72,7 @@ else() absl::flat_hash_set absl::function_ref absl::hash + absl::if_constexpr absl::layout absl::log_initialize absl::log_severity diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index ad2811ba7d615..55eca1fe1db33 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -338,6 +338,7 @@ cc_library( "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", "@com_google_absl//absl/synchronization", + "@com_google_absl//absl/utility:if_constexpr", ], ) diff --git a/src/google/protobuf/arena.h b/src/google/protobuf/arena.h index 07922a4812c99..c52219bf3df03 100644 --- a/src/google/protobuf/arena.h +++ b/src/google/protobuf/arena.h @@ -10,7 +10,10 @@ #ifndef GOOGLE_PROTOBUF_ARENA_H__ #define GOOGLE_PROTOBUF_ARENA_H__ +#include +#include #include +#include #include #include #include @@ -26,8 +29,11 @@ using type_info = ::type_info; #include #endif +#include "absl/base/attributes.h" #include "absl/log/absl_check.h" +#include "absl/utility/internal/if_constexpr.h" #include "google/protobuf/arena_align.h" +#include "google/protobuf/arena_allocation_policy.h" #include "google/protobuf/port.h" #include "google/protobuf/serial_arena.h" #include "google/protobuf/thread_safe_arena.h" @@ -48,6 +54,9 @@ class Message; // defined in message.h class MessageLite; template class Map; +namespace internal { +struct RepeatedFieldBase; +} // namespace internal namespace arena_metrics { @@ -210,7 +219,7 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8) Arena final { internal::ThreadSafeArena::kBlockHeaderSize + internal::ThreadSafeArena::kSerialArenaSize; - inline ~Arena() {} + inline ~Arena() = default; // API to create proto2 message objects on the arena. If the arena passed in // is nullptr, then a heap allocated object is returned. Type T must be a @@ -243,27 +252,31 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8) Arena final { return CreateMessageInternal(arena, std::forward(args)...); } - // API to create any objects on the arena. Note that only the object will - // be created on the arena; the underlying ptrs (in case of a proto2 message) - // will be still heap allocated. Proto messages should usually be allocated - // with CreateMessage() instead. + // API to create any objects on the arena. + // + // If an object is arena-constructable, this API behaves like + // Arena::CreateMessage. Arena constructable types are expected to accept + // Arena* as the first argument and one is implicitly added by the API. // - // Note that even if T satisfies the arena message construction protocol - // (InternalArenaConstructable_ trait and optional DestructorSkippable_ - // trait), as described above, this function does not follow the protocol; - // instead, it treats T as a black-box type, just as if it did not have these - // traits. Specifically, T's constructor arguments will always be only those - // passed to Create() -- no additional arena pointer is implicitly added. - // Furthermore, the destructor will always be called at arena destruction time - // (unless the destructor is trivial). Hence, from T's point of view, it is as - // if the object were allocated on the heap (except that the underlying memory - // is obtained from the arena). + // If the object is not arena-constructable, only the object will be created + // on the arena; the underlying ptrs will be still heap allocated. template PROTOBUF_NDEBUG_INLINE static T* Create(Arena* arena, Args&&... args) { - if (PROTOBUF_PREDICT_FALSE(arena == nullptr)) { - return new T(std::forward(args)...); - } - return new (arena->AllocateInternal()) T(std::forward(args)...); + return absl::utility_internal::IfConstexprElse< + is_arena_constructable::value>( + // Arena-constructable + [arena](auto&&... args) { + return CreateMessage(arena, std::forward(args)...); + }, + // Non arena-constructable + [arena](auto&&... args) { + if (PROTOBUF_PREDICT_FALSE(arena == nullptr)) { + return new T(std::forward(args)...); + } + return new (arena->AllocateInternal()) + T(std::forward(args)...); + }, + std::forward(args)...); } // API to delete any objects not on an arena. This can be used to safely diff --git a/src/google/protobuf/arena_unittest.cc b/src/google/protobuf/arena_unittest.cc index 78ccdfd427820..f1337b1c1f95d 100644 --- a/src/google/protobuf/arena_unittest.cc +++ b/src/google/protobuf/arena_unittest.cc @@ -450,6 +450,29 @@ DispatcherTestProto* Arena::CreateMessageInternal( return nullptr; } +TEST(ArenaTest, CreateArenaConstructable) { + TestAllTypes original; + TestUtil::SetAllFields(&original); + + Arena arena; + auto copied = Arena::Create(&arena, original); + + TestUtil::ExpectAllFieldsSet(*copied); + EXPECT_EQ(copied->GetArena(), &arena); + EXPECT_EQ(copied->optional_nested_message().GetArena(), &arena); +} + +TEST(ArenaTest, CreateRepeatedPtrField) { + Arena arena; + auto repeated = Arena::Create>(&arena); + TestUtil::SetAllFields(repeated->Add()); + + TestUtil::ExpectAllFieldsSet(repeated->Get(0)); + EXPECT_EQ(repeated->GetArena(), &arena); + EXPECT_EQ(repeated->Get(0).GetArena(), &arena); + EXPECT_EQ(repeated->Get(0).optional_nested_message().GetArena(), &arena); +} + TEST(ArenaTest, CreateMessageDispatchesToSpecialFunctions) { hook_called = ""; Arena::CreateMessage(nullptr); diff --git a/src/google/protobuf/proto3_arena_unittest.cc b/src/google/protobuf/proto3_arena_unittest.cc index 7a46ab6f2010b..3ada399c595b9 100644 --- a/src/google/protobuf/proto3_arena_unittest.cc +++ b/src/google/protobuf/proto3_arena_unittest.cc @@ -152,7 +152,7 @@ TEST(Proto3ArenaTest, GetArena) { // Tests message created by Arena::Create. auto* arena_message3 = Arena::Create(&arena); - EXPECT_EQ(nullptr, arena_message3->GetArena()); + EXPECT_EQ(&arena, arena_message3->GetArena()); } TEST(Proto3ArenaTest, GetArenaWithUnknown) {