Skip to content

Commit

Permalink
Have Arena::Create support arena constructible types
Browse files Browse the repository at this point in the history
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
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Nov 27, 2023
1 parent df8a449 commit 578e07e
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 20 deletions.
1 change: 1 addition & 0 deletions cmake/abseil-cpp.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/google/protobuf/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)

Expand Down
51 changes: 32 additions & 19 deletions src/google/protobuf/arena.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -26,8 +29,11 @@ using type_info = ::type_info;
#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"
Expand All @@ -48,6 +54,9 @@ class Message; // defined in message.h
class MessageLite;
template <typename Key, typename T>
class Map;
namespace internal {
struct RepeatedFieldBase;
} // namespace internal

namespace arena_metrics {

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -243,27 +252,31 @@ class PROTOBUF_EXPORT PROTOBUF_ALIGNAS(8) Arena final {
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.
//
// 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<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 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
Expand Down
23 changes: 23 additions & 0 deletions src/google/protobuf/arena_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,29 @@ DispatcherTestProto* Arena::CreateMessageInternal<DispatcherTestProto, int>(
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);
Expand Down
2 changes: 1 addition & 1 deletion src/google/protobuf/proto3_arena_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ TEST(Proto3ArenaTest, GetArena) {

// 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) {
Expand Down

0 comments on commit 578e07e

Please sign in to comment.