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<Foo>(&arena, &arena); // bad
```
This pattern is not supported and will break after this change. The following
is recommended instead.
```
auto foo = Arena::CreateMessage<Foo>(&arena); // recommended
auto foo = Arena::Create<Foo>(&arena); // after this change
```
PiperOrigin-RevId: 585709990
diff --git a/cmake/abseil-cpp.cmake b/cmake/abseil-cpp.cmake
index b50fb89..a4e9d22 100644
--- a/cmake/abseil-cpp.cmake
+++ b/cmake/abseil-cpp.cmake
@@ -72,6 +72,7 @@
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 ad2811b..55eca1f 100644
--- a/src/google/protobuf/BUILD.bazel
+++ b/src/google/protobuf/BUILD.bazel
@@ -338,6 +338,7 @@
"@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 07922a4..c52219b 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 <cstddef>
+#include <cstdint>
#include <limits>
+#include <new>
#include <string>
#include <type_traits>
#include <utility>
@@ -26,8 +29,11 @@
#include <typeinfo>
#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 MessageLite;
template <typename Key, typename T>
class Map;
+namespace internal {
+struct RepeatedFieldBase;
+} // namespace internal
namespace arena_metrics {
@@ -210,7 +219,7 @@
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 @@
return CreateMessageInternal<Type>(arena, std::forward<Args>(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<T>() instead.
+ // API to create any objects on the arena.
//
- // 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<T>() -- 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 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.
+ //
+ // 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 <typename T, typename... Args>
PROTOBUF_NDEBUG_INLINE static T* Create(Arena* arena, Args&&... args) {
- if (PROTOBUF_PREDICT_FALSE(arena == nullptr)) {
- return new T(std::forward<Args>(args)...);
- }
- return new (arena->AllocateInternal<T>()) T(std::forward<Args>(args)...);
+ return absl::utility_internal::IfConstexprElse<
+ is_arena_constructable<T>::value>(
+ // Arena-constructable
+ [arena](auto&&... args) {
+ return CreateMessage<T>(arena, std::forward<Args>(args)...);
+ },
+ // Non arena-constructable
+ [arena](auto&&... args) {
+ if (PROTOBUF_PREDICT_FALSE(arena == nullptr)) {
+ return new T(std::forward<Args>(args)...);
+ }
+ return new (arena->AllocateInternal<T>())
+ T(std::forward<Args>(args)...);
+ },
+ std::forward<Args>(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 78ccdfd..f1337b1 100644
--- a/src/google/protobuf/arena_unittest.cc
+++ b/src/google/protobuf/arena_unittest.cc
@@ -450,6 +450,29 @@
return nullptr;
}
+TEST(ArenaTest, CreateArenaConstructable) {
+ TestAllTypes original;
+ TestUtil::SetAllFields(&original);
+
+ Arena arena;
+ auto copied = Arena::Create<TestAllTypes>(&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<RepeatedPtrField<TestAllTypes>>(&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<DispatcherTestProto>(nullptr);
diff --git a/src/google/protobuf/proto3_arena_unittest.cc b/src/google/protobuf/proto3_arena_unittest.cc
index 7a46ab6..3ada399 100644
--- a/src/google/protobuf/proto3_arena_unittest.cc
+++ b/src/google/protobuf/proto3_arena_unittest.cc
@@ -152,7 +152,7 @@
// Tests message created by Arena::Create.
auto* arena_message3 = Arena::Create<TestAllTypes>(&arena);
- EXPECT_EQ(nullptr, arena_message3->GetArena());
+ EXPECT_EQ(&arena, arena_message3->GetArena());
}
TEST(Proto3ArenaTest, GetArenaWithUnknown) {
