Skip to content

Commit 578e07e

Browse files
protobuf-github-botcopybara-github
protobuf-github-bot
authored and
copybara-github
committed
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
1 parent df8a449 commit 578e07e

5 files changed

Lines changed: 58 additions & 20 deletions

File tree

‎cmake/abseil-cpp.cmake‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ else()
7272
absl::flat_hash_set
7373
absl::function_ref
7474
absl::hash
75+
absl::if_constexpr
7576
absl::layout
7677
absl::log_initialize
7778
absl::log_severity

‎src/google/protobuf/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,7 @@ cc_library(
338338
"@com_google_absl//absl/log:absl_check",
339339
"@com_google_absl//absl/log:absl_log",
340340
"@com_google_absl//absl/synchronization",
341+
"@com_google_absl//absl/utility:if_constexpr",
341342
],
342343
)
343344

‎src/google/protobuf/arena.h‎

Lines changed: 32 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@
1010
#ifndef GOOGLE_PROTOBUF_ARENA_H__
1111
#define GOOGLE_PROTOBUF_ARENA_H__
1212

13+
#include <cstddef>
14+
#include <cstdint>
1315
#include <limits>
16+
#include <new>
1417
#include <string>
1518
#include <type_traits>
1619
#include <utility>
@@ -26,8 +29,11 @@ using type_info = ::type_info;
2629
#include <typeinfo>
2730
#endif
2831

32+
#include "absl/base/attributes.h"
2933
#include "absl/log/absl_check.h"
34+
#include "absl/utility/internal/if_constexpr.h"
3035
#include "google/protobuf/arena_align.h"
36+
#include "google/protobuf/arena_allocation_policy.h"
3137
#include "google/protobuf/port.h"
3238
#include "google/protobuf/serial_arena.h"
3339
#include "google/protobuf/thread_safe_arena.h"
@@ -48,6 +54,9 @@ class Message; // defined in message.h
4854
class MessageLite;
4955
template <typename Key, typename T>
5056
class Map;
57+
namespace internal {
58+
struct RepeatedFieldBase;
59+
} // namespace internal
5160

5261
namespace arena_metrics {
5362

@@ -210,7 +219,7 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8) Arena final {
210219
internal::ThreadSafeArena::kBlockHeaderSize +
211220
internal::ThreadSafeArena::kSerialArenaSize;
212221

213-
inline ~Arena() {}
222+
inline ~Arena() = default;
214223

215224
// API to create proto2 message objects on the arena. If the arena passed in
216225
// 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 {
243252
return CreateMessageInternal<Type>(arena, std::forward<Args>(args)...);
244253
}
245254

246-
// API to create any objects on the arena. Note that only the object will
247-
// be created on the arena; the underlying ptrs (in case of a proto2 message)
248-
// will be still heap allocated. Proto messages should usually be allocated
249-
// with CreateMessage<T>() instead.
255+
// API to create any objects on the arena.
256+
//
257+
// If an object is arena-constructable, this API behaves like
258+
// Arena::CreateMessage. Arena constructable types are expected to accept
259+
// Arena* as the first argument and one is implicitly added by the API.
250260
//
251-
// Note that even if T satisfies the arena message construction protocol
252-
// (InternalArenaConstructable_ trait and optional DestructorSkippable_
253-
// trait), as described above, this function does not follow the protocol;
254-
// instead, it treats T as a black-box type, just as if it did not have these
255-
// traits. Specifically, T's constructor arguments will always be only those
256-
// passed to Create<T>() -- no additional arena pointer is implicitly added.
257-
// Furthermore, the destructor will always be called at arena destruction time
258-
// (unless the destructor is trivial). Hence, from T's point of view, it is as
259-
// if the object were allocated on the heap (except that the underlying memory
260-
// is obtained from the arena).
261+
// If the object is not arena-constructable, only the object will be created
262+
// on the arena; the underlying ptrs will be still heap allocated.
261263
template <typename T, typename... Args>
262264
PROTOBUF_NDEBUG_INLINE static T* Create(Arena* arena, Args&&... args) {
263-
if (PROTOBUF_PREDICT_FALSE(arena == nullptr)) {
264-
return new T(std::forward<Args>(args)...);
265-
}
266-
return new (arena->AllocateInternal<T>()) T(std::forward<Args>(args)...);
265+
return absl::utility_internal::IfConstexprElse<
266+
is_arena_constructable<T>::value>(
267+
// Arena-constructable
268+
[arena](auto&&... args) {
269+
return CreateMessage<T>(arena, std::forward<Args>(args)...);
270+
},
271+
// Non arena-constructable
272+
[arena](auto&&... args) {
273+
if (PROTOBUF_PREDICT_FALSE(arena == nullptr)) {
274+
return new T(std::forward<Args>(args)...);
275+
}
276+
return new (arena->AllocateInternal<T>())
277+
T(std::forward<Args>(args)...);
278+
},
279+
std::forward<Args>(args)...);
267280
}
268281

269282
// API to delete any objects not on an arena. This can be used to safely

‎src/google/protobuf/arena_unittest.cc‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,29 @@ DispatcherTestProto* Arena::CreateMessageInternal<DispatcherTestProto, int>(
450450
return nullptr;
451451
}
452452

453+
TEST(ArenaTest, CreateArenaConstructable) {
454+
TestAllTypes original;
455+
TestUtil::SetAllFields(&original);
456+
457+
Arena arena;
458+
auto copied = Arena::Create<TestAllTypes>(&arena, original);
459+
460+
TestUtil::ExpectAllFieldsSet(*copied);
461+
EXPECT_EQ(copied->GetArena(), &arena);
462+
EXPECT_EQ(copied->optional_nested_message().GetArena(), &arena);
463+
}
464+
465+
TEST(ArenaTest, CreateRepeatedPtrField) {
466+
Arena arena;
467+
auto repeated = Arena::Create<RepeatedPtrField<TestAllTypes>>(&arena);
468+
TestUtil::SetAllFields(repeated->Add());
469+
470+
TestUtil::ExpectAllFieldsSet(repeated->Get(0));
471+
EXPECT_EQ(repeated->GetArena(), &arena);
472+
EXPECT_EQ(repeated->Get(0).GetArena(), &arena);
473+
EXPECT_EQ(repeated->Get(0).optional_nested_message().GetArena(), &arena);
474+
}
475+
453476
TEST(ArenaTest, CreateMessageDispatchesToSpecialFunctions) {
454477
hook_called = "";
455478
Arena::CreateMessage<DispatcherTestProto>(nullptr);

‎src/google/protobuf/proto3_arena_unittest.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ TEST(Proto3ArenaTest, GetArena) {
152152

153153
// Tests message created by Arena::Create.
154154
auto* arena_message3 = Arena::Create<TestAllTypes>(&arena);
155-
EXPECT_EQ(nullptr, arena_message3->GetArena());
155+
EXPECT_EQ(&arena, arena_message3->GetArena());
156156
}
157157

158158
TEST(Proto3ArenaTest, GetArenaWithUnknown) {

0 commit comments

Comments
 (0)