From 6e1eba6da7cab6a9f3c44616048873be6dfde4bb Mon Sep 17 00:00:00 2001 From: Justin King Date: Tue, 11 Mar 2025 09:52:22 -0700 Subject: [PATCH] Switch from `cel::Shared` to `cel::Owned` PiperOrigin-RevId: 735791355 --- common/BUILD | 5 +- common/allocator.h | 35 +- common/arena.h | 79 +- common/internal/BUILD | 2 +- common/internal/byte_string.cc | 5 +- common/internal/byte_string.h | 31 +- common/internal/byte_string_test.cc | 15 +- common/internal/reference_count.h | 5 +- common/legacy_value.cc | 49 +- common/legacy_value.h | 10 +- common/memory.h | 84 +- common/value.cc | 17 - common/value.h | 111 +- common/values/bytes_value.h | 23 +- common/values/custom_list_value.cc | 8 +- common/values/custom_list_value.h | 21 +- common/values/custom_map_value.cc | 8 +- common/values/custom_map_value.h | 17 +- common/values/custom_struct_value.cc | 5 +- common/values/custom_struct_value.h | 17 +- common/values/error_value.h | 9 + common/values/list_value.h | 11 +- common/values/list_value_builder.h | 6 +- common/values/map_value.h | 11 +- common/values/map_value_builder.h | 7 +- common/values/message_value.h | 13 + common/values/opaque_value.cc | 5 +- common/values/opaque_value.h | 17 +- common/values/optional_value.cc | 29 +- common/values/optional_value.h | 13 +- common/values/parsed_json_list_value.h | 9 + common/values/parsed_json_map_value.h | 9 + common/values/parsed_map_field_value.h | 9 + common/values/parsed_message_value.h | 9 + common/values/parsed_repeated_field_value.h | 9 + common/values/string_value.h | 24 +- common/values/struct_value.h | 17 +- common/values/value_builder.cc | 1087 ++++++------------- eval/tests/modern_benchmark_test.cc | 7 +- internal/BUILD | 9 + internal/manual.h | 91 ++ 41 files changed, 895 insertions(+), 1053 deletions(-) create mode 100644 internal/manual.h diff --git a/common/BUILD b/common/BUILD index 055d3f334..5861e3db8 100644 --- a/common/BUILD +++ b/common/BUILD @@ -585,6 +585,7 @@ cc_library( deps = [ ":allocator", ":any", + ":arena", ":casting", ":kind", ":memory", @@ -595,7 +596,6 @@ cc_library( ":value_kind", "//base:attributes", "//common/internal:byte_string", - "//common/internal:reference_count", "//eval/internal:cel_value_equal", "//eval/public:cel_value", "//eval/public:message_wrapper", @@ -609,6 +609,7 @@ cc_library( "//extensions/protobuf/internal:qualify", "//internal:casts", "//internal:json", + "//internal:manual", "//internal:message_equality", "//internal:number", "//internal:protobuf_runtime_version", @@ -696,7 +697,6 @@ cc_library( hdrs = ["arena.h"], deps = [ "@com_google_absl//absl/base:nullability", - "@com_google_absl//absl/meta:type_traits", "@com_google_protobuf//:protobuf", ], ) @@ -712,6 +712,7 @@ cc_library( hdrs = ["allocator.h"], deps = [ ":arena", + ":data", "//internal:new", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:nullability", diff --git a/common/allocator.h b/common/allocator.h index 8237d677f..779d4bace 100644 --- a/common/allocator.h +++ b/common/allocator.h @@ -27,6 +27,7 @@ #include "absl/log/die_if_null.h" #include "absl/numeric/bits.h" #include "common/arena.h" +#include "common/data.h" #include "internal/new.h" #include "google/protobuf/arena.h" @@ -287,9 +288,29 @@ class ArenaAllocator { // called. template ABSL_MUST_USE_RESULT T* new_object(Args&&... args) { - auto* object = google::protobuf::Arena::Create>( - arena(), std::forward(args)...); - if constexpr (IsArenaConstructible::value) { + using U = std::remove_const_t; + U* object; + if constexpr (google::protobuf::Arena::is_arena_constructable::value) { + // Classes derived from `cel::Data` are manually allocated and constructed + // as those class support determining whether the destructor is skippable + // at runtime. + object = google::protobuf::Arena::Create(arena(), std::forward(args)...); + } else { + if constexpr (ArenaTraits<>::constructible()) { + object = ::new (static_cast(arena()->AllocateAligned( + sizeof(U), alignof(U)))) U(arena(), std::forward(args)...); + } else { + object = ::new (static_cast(arena()->AllocateAligned( + sizeof(U), alignof(U)))) U(std::forward(args)...); + } + if constexpr (!ArenaTraits<>::always_trivially_destructible()) { + if (!ArenaTraits<>::trivially_destructible(*object)) { + arena()->OwnDestructor(object); + } + } + } + if constexpr (google::protobuf::Arena::is_arena_constructable::value || + std::is_base_of_v) { ABSL_DCHECK_EQ(object->GetArena(), arena()); } return object; @@ -299,8 +320,10 @@ class ArenaAllocator { // memory, `p` must have been previously returned by `new_object`. template void delete_object(T* p) noexcept { + using U = std::remove_const_t; ABSL_DCHECK(p != nullptr); - if constexpr (IsArenaConstructible::value) { + if constexpr (google::protobuf::Arena::is_arena_constructable::value || + std::is_base_of_v) { ABSL_DCHECK_EQ(p->GetArena(), arena()); } } @@ -359,13 +382,13 @@ class ArenaAllocator : public ArenaAllocator { template void construct(U* p, Args&&... args) { - static_assert(!IsArenaConstructible::value); + static_assert(!google::protobuf::Arena::is_arena_constructable::value); ::new (static_cast(p)) U(std::forward(args)...); } template void destroy(U* p) noexcept { - static_assert(!IsArenaConstructible::value); + static_assert(!google::protobuf::Arena::is_arena_constructable::value); std::destroy_at(p); } }; diff --git a/common/arena.h b/common/arena.h index 4be983767..21ab8ef40 100644 --- a/common/arena.h +++ b/common/arena.h @@ -16,38 +16,95 @@ #define THIRD_PARTY_CEL_CPP_COMMON_ARENA_H_ #include +#include #include "absl/base/nullability.h" -#include "absl/meta/type_traits.h" #include "google/protobuf/arena.h" namespace cel { -template -using IsArenaConstructible = google::protobuf::Arena::is_arena_constructable; +template +struct ArenaTraits; + +namespace common_internal { template -using IsArenaDestructorSkippable = - absl::conjunction, - google::protobuf::Arena::is_destructor_skippable>; +struct AssertArenaType : std::false_type { + static_assert(!std::is_void_v, "T must not be void"); + static_assert(!std::is_reference_v, "T must not be a reference"); + static_assert(!std::is_volatile_v, "T must not be volatile qualified"); + static_assert(!std::is_const_v, "T must not be const qualified"); + static_assert(!std::is_array_v, "T must not be an array"); +}; -namespace common_internal { +template +struct ArenaTraitsConstructible { + using type = std::false_type; +}; template -std::enable_if_t::value, absl::Nullable> -GetArena(const T* ptr) { +struct ArenaTraitsConstructible< + T, std::void_t::constructible)>> { + using type = typename ArenaTraits::constructible; +}; + +template +std::enable_if_t::value, + absl::Nullable> +GetArena(absl::Nullable ptr) { return ptr != nullptr ? ptr->GetArena() : nullptr; } template -std::enable_if_t::value, +std::enable_if_t::value, absl::Nullable> -GetArena([[maybe_unused]] const T* ptr) { +GetArena([[maybe_unused]] absl::Nullable ptr) { return nullptr; } +template +struct HasArenaTraitsTriviallyDestructible : std::false_type {}; + +template +struct HasArenaTraitsTriviallyDestructible< + T, std::void_t::trivially_destructible( + std::declval()))>> : std::true_type {}; + } // namespace common_internal +template <> +struct ArenaTraits { + template + using constructible = std::disjunction< + typename common_internal::AssertArenaType::type, + typename common_internal::ArenaTraitsConstructible::type>; + + template + using always_trivially_destructible = + std::disjunction::type, + std::is_trivially_destructible>; + + template + static bool trivially_destructible(const U& obj) { + static_assert(!std::is_void_v, "T must not be void"); + static_assert(!std::is_reference_v, "T must not be a reference"); + static_assert(!std::is_volatile_v, "T must not be volatile qualified"); + static_assert(!std::is_const_v, "T must not be const qualified"); + static_assert(!std::is_array_v, "T must not be an array"); + + if constexpr (always_trivially_destructible()) { + return true; + } else if constexpr (google::protobuf::Arena::is_destructor_skippable::value) { + return obj.GetArena() != nullptr; + } else if constexpr (common_internal::HasArenaTraitsTriviallyDestructible< + U>::value) { + return ArenaTraits::trivially_destructible(obj); + } else { + return false; + } + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_ARENA_H_ diff --git a/common/internal/BUILD b/common/internal/BUILD index 49f59b9d2..0ce85ff7e 100644 --- a/common/internal/BUILD +++ b/common/internal/BUILD @@ -32,7 +32,6 @@ cc_library( srcs = ["reference_count.cc"], hdrs = ["reference_count.h"], deps = [ - "//common:arena", "//common:data", "//internal:new", "@com_google_absl//absl/base:core_headers", @@ -69,6 +68,7 @@ cc_library( ":metadata", ":reference_count", "//common:allocator", + "//common:arena", "//common:memory", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:nullability", diff --git a/common/internal/byte_string.cc b/common/internal/byte_string.cc index a4b45d7f9..0e2c19a65 100644 --- a/common/internal/byte_string.cc +++ b/common/internal/byte_string.cc @@ -921,7 +921,7 @@ void ByteString::SetLarge(absl::Cord&& cord) { ::new (static_cast(&rep_.large.data[0])) absl::Cord(std::move(cord)); } -absl::string_view LegacyByteString(const ByteString& string, +absl::string_view LegacyByteString(const ByteString& string, bool stable, absl::Nonnull arena) { ABSL_DCHECK(arena != nullptr); if (string.empty()) { @@ -936,6 +936,9 @@ absl::string_view LegacyByteString(const ByteString& string, return string.GetMedium(); } } + if (stable && kind == ByteStringKind::kSmall) { + return string.GetSmall(); + } absl::Nonnull result = google::protobuf::Arena::Create(arena); switch (kind) { diff --git a/common/internal/byte_string.h b/common/internal/byte_string.h index ed7381872..f2c11589e 100644 --- a/common/internal/byte_string.h +++ b/common/internal/byte_string.h @@ -33,6 +33,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "common/allocator.h" +#include "common/arena.h" #include "common/internal/metadata.h" #include "common/internal/reference_count.h" #include "common/memory.h" @@ -45,8 +46,6 @@ class BytesValueOutputStream; namespace common_internal { -class TrivialValue; - // absl::Cord is trivially relocatable IFF we are not using ASan or MSan. When // using ASan or MSan absl::Cord will poison/unpoison its inline storage. #if defined(ABSL_HAVE_ADDRESS_SANITIZER) || defined(ABSL_HAVE_MEMORY_SANITIZER) @@ -155,7 +154,11 @@ union CEL_COMMON_INTERNAL_BYTE_STRING_TRIVIAL_ABI ByteStringRep final { LargeByteStringRep large; }; -absl::string_view LegacyByteString(const ByteString& string, +// Returns a `absl::string_view` from `ByteString`, using `arena` to make memory +// allocations if necessary. `stable` indicates whether `cel::Value` is in a +// location where it will not be moved, so that inline string/bytes storage can +// be referenced. +absl::string_view LegacyByteString(const ByteString& string, bool stable, absl::Nonnull arena); // `ByteString` is an vocabulary type capable of representing copy-on-write @@ -328,11 +331,12 @@ ByteString final { private: friend class ByteStringView; friend struct ByteStringTestFriend; - friend class TrivialValue; friend class cel::BytesValueInputStream; friend class cel::BytesValueOutputStream; friend absl::string_view LegacyByteString( - const ByteString& string, absl::Nonnull arena); + const ByteString& string, bool stable, + absl::Nonnull arena); + friend struct cel::ArenaTraits; static ByteString Borrowed(Borrower borrower, absl::string_view string @@ -804,6 +808,23 @@ inline ByteString& ByteString::operator=(ByteStringView other) { } // namespace common_internal +template <> +struct ArenaTraits { + using constructible = std::true_type; + + static bool trivially_destructible( + const common_internal::ByteString& byte_string) { + switch (byte_string.GetKind()) { + case common_internal::ByteStringKind::kSmall: + return true; + case common_internal::ByteStringKind::kMedium: + return byte_string.GetMediumReferenceCount() == nullptr; + case common_internal::ByteStringKind::kLarge: + return false; + } + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_INTERNAL_BYTE_STRING_H_ diff --git a/common/internal/byte_string_test.cc b/common/internal/byte_string_test.cc index 39aca59b2..dbad555e8 100644 --- a/common/internal/byte_string_test.cc +++ b/common/internal/byte_string_test.cc @@ -1142,19 +1142,28 @@ TEST_P(ByteStringTest, CloneLarge) { TEST_P(ByteStringTest, LegacyByteStringSmall) { google::protobuf::Arena arena; ByteString byte_string = ByteString(GetAllocator(), GetSmallStringView()); - EXPECT_EQ(LegacyByteString(byte_string, &arena), GetSmallStringView()); + EXPECT_EQ(LegacyByteString(byte_string, /*stable=*/false, &arena), + GetSmallStringView()); + EXPECT_EQ(LegacyByteString(byte_string, /*stable=*/true, &arena), + GetSmallStringView()); } TEST_P(ByteStringTest, LegacyByteStringMedium) { google::protobuf::Arena arena; ByteString byte_string = ByteString(GetAllocator(), GetMediumStringView()); - EXPECT_EQ(LegacyByteString(byte_string, &arena), GetMediumStringView()); + EXPECT_EQ(LegacyByteString(byte_string, /*stable=*/false, &arena), + GetMediumStringView()); + EXPECT_EQ(LegacyByteString(byte_string, /*stable=*/true, &arena), + GetMediumStringView()); } TEST_P(ByteStringTest, LegacyByteStringLarge) { google::protobuf::Arena arena; ByteString byte_string = ByteString(GetAllocator(), GetMediumOrLargeCord()); - EXPECT_EQ(LegacyByteString(byte_string, &arena), GetMediumOrLargeCord()); + EXPECT_EQ(LegacyByteString(byte_string, /*stable=*/false, &arena), + GetMediumOrLargeCord()); + EXPECT_EQ(LegacyByteString(byte_string, /*stable=*/true, &arena), + GetMediumOrLargeCord()); } TEST_P(ByteStringTest, HashValue) { diff --git a/common/internal/reference_count.h b/common/internal/reference_count.h index 4bf8bc8a2..803905d31 100644 --- a/common/internal/reference_count.h +++ b/common/internal/reference_count.h @@ -32,7 +32,6 @@ #include "absl/base/optimization.h" #include "absl/log/absl_check.h" #include "absl/strings/string_view.h" -#include "common/arena.h" #include "common/data.h" #include "google/protobuf/arena.h" #include "google/protobuf/message_lite.h" @@ -216,7 +215,7 @@ extern template class DeletingReferenceCount; template absl::Nonnull MakeDeletingReferenceCount( absl::Nonnull to_delete) { - if constexpr (IsArenaConstructible::value) { + if constexpr (google::protobuf::Arena::is_arena_constructable::value) { ABSL_DCHECK_EQ(to_delete->GetArena(), nullptr); } if constexpr (std::is_base_of_v) { @@ -237,7 +236,7 @@ MakeEmplacedReferenceCount(Args&&... args) { U* pointer; auto* const refcount = new EmplacedReferenceCount(pointer, std::forward(args)...); - if constexpr (IsArenaConstructible::value) { + if constexpr (google::protobuf::Arena::is_arena_constructable::value) { ABSL_DCHECK_EQ(pointer->GetArena(), nullptr); } if constexpr (std::is_base_of_v) { diff --git a/common/legacy_value.cc b/common/legacy_value.cc index c441563b5..36c68813f 100644 --- a/common/legacy_value.cc +++ b/common/legacy_value.cc @@ -36,7 +36,6 @@ #include "absl/types/span.h" #include "absl/types/variant.h" #include "base/attribute.h" -#include "common/allocator.h" #include "common/casting.h" #include "common/kind.h" #include "common/memory.h" @@ -261,35 +260,37 @@ CelValue LegacyTrivialMapValue(absl::Nonnull arena, } // namespace -google::api::expr::runtime::CelValue LegacyTrivialValue( - absl::Nonnull arena, const TrivialValue& value) { - switch (value->kind()) { +google::api::expr::runtime::CelValue UnsafeLegacyValue( + const Value& value, bool stable, absl::Nonnull arena) { + switch (value.kind()) { case ValueKind::kNull: return CelValue::CreateNull(); case ValueKind::kBool: - return CelValue::CreateBool(value->GetBool().NativeValue()); + return CelValue::CreateBool(value.GetBool()); case ValueKind::kInt: - return CelValue::CreateInt64(value->GetInt().NativeValue()); + return CelValue::CreateInt64(value.GetInt()); case ValueKind::kUint: - return CelValue::CreateUint64(value->GetUint().NativeValue()); + return CelValue::CreateUint64(value.GetUint()); case ValueKind::kDouble: - return CelValue::CreateDouble(value->GetDouble().NativeValue()); + return CelValue::CreateDouble(value.GetDouble()); case ValueKind::kString: - return CelValue::CreateStringView(value.ToString()); + return CelValue::CreateStringView( + LegacyStringValue(value.GetString(), stable, arena)); case ValueKind::kBytes: - return CelValue::CreateBytesView(value.ToBytes()); + return CelValue::CreateBytesView( + LegacyBytesValue(value.GetBytes(), stable, arena)); case ValueKind::kStruct: - return LegacyTrivialStructValue(arena, *value); + return LegacyTrivialStructValue(arena, value); case ValueKind::kDuration: - return CelValue::CreateDuration(value->GetDuration().NativeValue()); + return CelValue::CreateDuration(value.GetDuration().ToDuration()); case ValueKind::kTimestamp: - return CelValue::CreateTimestamp(value->GetTimestamp().NativeValue()); + return CelValue::CreateTimestamp(value.GetTimestamp().ToTime()); case ValueKind::kList: - return LegacyTrivialListValue(arena, *value); + return LegacyTrivialListValue(arena, value); case ValueKind::kMap: - return LegacyTrivialMapValue(arena, *value); + return LegacyTrivialMapValue(arena, value); case ValueKind::kType: - return CelValue::CreateCelTypeView(value->GetType().name()); + return CelValue::CreateCelTypeView(value.GetType().name()); default: // Everything else is unsupported. return CelValue::CreateError(google::protobuf::Arena::Create( @@ -1019,11 +1020,11 @@ absl::StatusOr LegacyValue( return CelValue::CreateDouble( Cast(modern_value).NativeValue()); case ValueKind::kString: - return CelValue::CreateStringView( - common_internal::LegacyStringValue(modern_value.GetString(), arena)); + return CelValue::CreateStringView(common_internal::LegacyStringValue( + modern_value.GetString(), /*stable=*/false, arena)); case ValueKind::kBytes: - return CelValue::CreateBytesView( - common_internal::LegacyBytesValue(modern_value.GetBytes(), arena)); + return CelValue::CreateBytesView(common_internal::LegacyBytesValue( + modern_value.GetBytes(), /*stable=*/false, arena)); case ValueKind::kStruct: return common_internal::LegacyTrivialStructValue(arena, modern_value); case ValueKind::kDuration: @@ -1123,11 +1124,11 @@ absl::StatusOr ToLegacyValue( case ValueKind::kDouble: return CelValue::CreateDouble(Cast(value).NativeValue()); case ValueKind::kString: - return CelValue::CreateStringView( - common_internal::LegacyStringValue(value.GetString(), arena)); + return CelValue::CreateStringView(common_internal::LegacyStringValue( + value.GetString(), /*stable=*/false, arena)); case ValueKind::kBytes: - return CelValue::CreateBytesView( - common_internal::LegacyBytesValue(value.GetBytes(), arena)); + return CelValue::CreateBytesView(common_internal::LegacyBytesValue( + value.GetBytes(), /*stable=*/false, arena)); case ValueKind::kStruct: return common_internal::LegacyTrivialStructValue(arena, value); case ValueKind::kDuration: diff --git a/common/legacy_value.h b/common/legacy_value.h index f1f5928ba..35f0e24a9 100644 --- a/common/legacy_value.h +++ b/common/legacy_value.h @@ -46,10 +46,12 @@ absl::StatusOr LegacyValue( namespace common_internal { -// Converts `Value` to `google::api::expr::runtime::CelValue`, or returns an -// error value. -google::api::expr::runtime::CelValue LegacyTrivialValue( - absl::Nonnull arena, const TrivialValue& value); +// Convert a `cel::Value` to `google::api::expr::runtime::CelValue`, using +// `arena` to make memory allocations if necessary. `stable` indicates whether +// `cel::Value` is in a location where it will not be moved, so that inline +// string/bytes storage can be referenced. +google::api::expr::runtime::CelValue UnsafeLegacyValue( + const Value& value, bool stable, absl::Nonnull arena); } // namespace common_internal diff --git a/common/memory.h b/common/memory.h index e821a074a..83e436a7f 100644 --- a/common/memory.h +++ b/common/memory.h @@ -36,7 +36,7 @@ #include "common/native_type.h" #include "common/reference_count.h" #include "internal/exceptions.h" -#include "internal/to_address.h" +#include "internal/to_address.h" // IWYU pragma: keep #include "google/protobuf/arena.h" namespace cel { @@ -260,6 +260,7 @@ class ABSL_ATTRIBUTE_TRIVIAL_ABI [[nodiscard]] Owner final { common_internal::OwnerRelease(Owner owner) noexcept; friend absl::Nullable common_internal::BorrowerRelease(Borrower borrower) noexcept; + friend struct ArenaTraits; constexpr explicit Owner(uintptr_t ptr) noexcept : ptr_(ptr) {} @@ -332,6 +333,13 @@ inline absl::Nullable OwnerRelease( } // namespace common_internal +template <> +struct ArenaTraits { + static bool trivially_destructible(const Owner& owner) { + return !Owner::IsReferenceCount(owner.ptr_); + } +}; + // `Borrower` represents a reference to some borrowed data, where the data has // at least one owner. When using reference counting, `Borrower` does not // participate in incrementing/decrementing the reference count. Thus `Borrower` @@ -621,6 +629,7 @@ class ABSL_ATTRIBUTE_TRIVIAL_ABI [[nodiscard]] Unique final { friend class ReferenceCountingMemoryManager; friend class PoolingMemoryManager; friend struct std::pointer_traits>; + friend struct ArenaTraits>; Unique(T* ptr, uintptr_t arena) noexcept : ptr_(ptr), arena_(arena) {} @@ -642,8 +651,9 @@ class ABSL_ATTRIBUTE_TRIVIAL_ABI [[nodiscard]] Unique final { if ((arena_ & common_internal::kUniqueArenaBits) == common_internal::kUniqueArenaUnownedBit) { // We never registered the destructor, call it if necessary. - if constexpr (!IsArenaDestructorSkippable::value) { - ptr_->~T(); + if constexpr (!std::is_trivially_destructible_v && + !google::protobuf::Arena::is_destructor_skippable::value) { + std::destroy_at(ptr_); } } } else { @@ -653,7 +663,8 @@ class ABSL_ATTRIBUTE_TRIVIAL_ABI [[nodiscard]] Unique final { } void PreRelease() noexcept { - if constexpr (!IsArenaDestructorSkippable::value) { + if constexpr (!std::is_trivially_destructible_v && + !google::protobuf::Arena::is_destructor_skippable::value) { if (static_cast(*this) && (arena_ & common_internal::kUniqueArenaBits) == common_internal::kUniqueArenaUnownedBit) { @@ -692,24 +703,35 @@ Unique(T*) -> Unique; template Unique AllocateUnique(Allocator<> allocator, Args&&... args) { - T* object; - auto* arena = allocator.arena(); + using U = std::remove_cv_t; + static_assert(!std::is_reference_v, "T must not be a reference"); + static_assert(!std::is_array_v, "T must not be an array"); + + U* object; + absl::Nullable arena = allocator.arena(); bool unowned; - if constexpr (IsArenaConstructible::value) { - object = google::protobuf::Arena::Create(arena, std::forward(args)...); + if constexpr (google::protobuf::Arena::is_arena_constructable::value) { + object = google::protobuf::Arena::Create(arena, std::forward(args)...); // For arena-compatible proto types, let the Arena::Create handle // registering the destructor call. // Otherwise, Unique retains a pointer to the owning arena so it may // conditionally register T::~T depending on usage. unowned = false; } else { - void* p = allocator.allocate_bytes(sizeof(T), alignof(T)); - CEL_INTERNAL_TRY { object = ::new (p) T(std::forward(args)...); } + void* p = allocator.allocate_bytes(sizeof(U), alignof(U)); + CEL_INTERNAL_TRY { + if constexpr (ArenaTraits<>::constructible()) { + object = ::new (p) U(arena, std::forward(args)...); + } else { + object = ::new (p) U(std::forward(args)...); + } + } CEL_INTERNAL_CATCH_ANY { - allocator.deallocate_bytes(p, sizeof(T), alignof(T)); + allocator.deallocate_bytes(p, sizeof(U), alignof(U)); CEL_INTERNAL_RETHROW; } - unowned = arena != nullptr; + unowned = + arena != nullptr && !ArenaTraits<>::trivially_destructible(*object); } return Unique(object, arena, unowned); } @@ -764,6 +786,14 @@ struct pointer_traits> { namespace cel { +template +struct ArenaTraits> { + static bool trivially_destructible(const Unique& unique) { + return unique.arena_ != 0 && + (unique.arena_ & common_internal::kUniqueArenaBits) == 0; + } +}; + // `Owned` points to an object which was allocated using `Allocator<>` or // `Allocator`. It has co-ownership over the object. `T` must meet the named // requirement `ArenaConstructable`. @@ -905,6 +935,7 @@ class ABSL_ATTRIBUTE_TRIVIAL_ABI [[nodiscard]] Owned final { template friend Owned common_internal::WrapEternal(const U* value); friend struct std::pointer_traits>; + friend struct ArenaTraits>; Owned(T* value, Owner owner) noexcept : value_(value), owner_(std::move(owner)) {} @@ -946,6 +977,13 @@ struct pointer_traits> { namespace cel { +template +struct ArenaTraits> { + static bool trivially_destructible(const Owned& owned) { + return ArenaTraits<>::trivially_destructible(owned.owner_); + } +}; + template Owner::Owner(const Owned& owned) noexcept : Owner(owned.owner_) {} @@ -989,22 +1027,26 @@ bool operator!=(std::nullptr_t, const Owned& rhs) noexcept { template Owned AllocateShared(Allocator<> allocator, Args&&... args) { - static_assert(IsArenaConstructible>::value, - "T must be arena constructable"); - T* object; + using U = std::remove_cv_t; + static_assert(!std::is_reference_v, "T must not be a reference"); + static_assert(!std::is_array_v, "T must not be an array"); + + U* object; Owner owner; - if (allocator.arena() != nullptr) { - object = allocator.new_object(std::forward(args)...); - owner.ptr_ = reinterpret_cast(allocator.arena()) | + if (absl::Nullable arena = allocator.arena(); + arena != nullptr) { + object = ArenaAllocator(arena).template new_object( + std::forward(args)...); + owner.ptr_ = reinterpret_cast(arena) | common_internal::kMetadataOwnerArenaBit; } else { const common_internal::ReferenceCount* refcount; - std::tie(object, refcount) = common_internal::MakeEmplacedReferenceCount( + std::tie(object, refcount) = common_internal::MakeEmplacedReferenceCount( std::forward(args)...); owner.ptr_ = reinterpret_cast(refcount) | common_internal::kMetadataOwnerReferenceCountBit; } - return Owned(object, std::move(owner)); + return Owned(object, std::move(owner)); } template @@ -1889,8 +1931,8 @@ class MemoryManager final { absl::Nullable arena() const noexcept { return arena_; } - // NOLINTNEXTLINE(google-explicit-constructor) template + // NOLINTNEXTLINE(google-explicit-constructor) operator Allocator() const { return arena(); } diff --git a/common/value.cc b/common/value.cc index 9e5c5fab3..9130237eb 100644 --- a/common/value.cc +++ b/common/value.cc @@ -2372,21 +2372,4 @@ bool operator==(DoubleValue lhs, UintValue rhs) { internal::Number::FromUint64(rhs.NativeValue()); } -namespace common_internal { - -TrivialValue MakeTrivialValue(const Value& value, - absl::Nonnull arena) { - return TrivialValue(value.Clone(arena)); -} - -absl::string_view TrivialValue::ToString() const { - return (*this)->GetString().value_.AsStringView(); -} - -absl::string_view TrivialValue::ToBytes() const { - return (*this)->GetBytes().value_.AsStringView(); -} - -} // namespace common_internal - } // namespace cel diff --git a/common/value.h b/common/value.h index 1cec4306b..68e6b7243 100644 --- a/common/value.h +++ b/common/value.h @@ -19,7 +19,6 @@ #include #include #include -#include #include #include #include @@ -39,6 +38,7 @@ #include "absl/utility/utility.h" #include "base/attribute.h" #include "common/allocator.h" +#include "common/arena.h" #include "common/memory.h" #include "common/native_type.h" #include "common/optional_ref.h" @@ -463,10 +463,8 @@ class Value final : private common_internal::ValueMixin { bool IsZeroValue() const; - // Clones the value to another allocator, if necessary. For compatible - // allocators, no allocation is performed. The exact logic for whether - // allocators are compatible is a little fuzzy at the moment, so avoid calling - // this function as it should be considered experimental. + // Clones the value to another arena, if necessary, such that the lifetime of + // the value is tied to the arena. Value Clone(absl::Nonnull arena) const; friend void swap(Value& lhs, Value& rhs) noexcept; @@ -2566,6 +2564,7 @@ class Value final : private common_internal::ValueMixin { friend common_internal::LegacyStructValue common_internal::GetLegacyStructValue(const Value& value); friend class common_internal::ValueMixin; + friend struct ArenaTraits; constexpr bool IsValid() const { return !absl::holds_alternative(variant_); @@ -2623,20 +2622,15 @@ struct NativeTypeTraits final { }, value.variant_); } +}; - static bool SkipDestructor(const Value& value) { +template <> +struct ArenaTraits { + static bool trivially_destructible(const Value& value) { value.AssertIsValid(); return absl::visit( [](const auto& alternative) -> bool { - if constexpr (std::is_same_v< - absl::remove_cvref_t, - absl::monostate>) { - // In optimized builds, we just say we should skip the destructor. - // In debug builds we cannot reach here. - return true; - } else { - return NativeType::SkipDestructor(alternative); - } + return ArenaTraits<>::trivially_destructible(alternative); }, value.variant_); } @@ -2856,6 +2850,8 @@ absl::StatusOr> StructValueMixin::Qualify( } // namespace common_internal +using ValueIteratorPtr = std::unique_ptr; + inline absl::StatusOr ValueIterator::Next( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, @@ -2934,91 +2930,6 @@ using RepeatedFieldAccessor = absl::StatusOr RepeatedFieldAccessorFor( absl::Nonnull field); -// Wrapper around `Value`, providing the same API as `TrivialValue`. -class NonTrivialValue final { - public: - NonTrivialValue() = default; - NonTrivialValue(const NonTrivialValue&) = default; - NonTrivialValue(NonTrivialValue&&) = default; - NonTrivialValue& operator=(const NonTrivialValue&) = default; - NonTrivialValue& operator=(NonTrivialValue&&) = default; - - explicit NonTrivialValue(const Value& other) : value_(other) {} - - explicit NonTrivialValue(Value&& other) : value_(std::move(other)) {} - - absl::Nonnull get() { return std::addressof(value_); } - - absl::Nonnull get() const { return std::addressof(value_); } - - Value& operator*() ABSL_ATTRIBUTE_LIFETIME_BOUND { return *get(); } - - const Value& operator*() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - return *get(); - } - - absl::Nonnull operator->() { return get(); } - - absl::Nonnull operator->() const { return get(); } - - friend void swap(NonTrivialValue& lhs, NonTrivialValue& rhs) noexcept { - using std::swap; - swap(lhs.value_, rhs.value_); - } - - private: - Value value_; -}; - -class TrivialValue; - -TrivialValue MakeTrivialValue(const Value& value, - absl::Nonnull arena); - -// Wrapper around `Value` which makes it trivial, providing the same API as -// `NonTrivialValue`. -class TrivialValue final { - public: - TrivialValue() : TrivialValue(Value()) {} - TrivialValue(const TrivialValue&) = default; - TrivialValue(TrivialValue&&) = default; - TrivialValue& operator=(const TrivialValue&) = default; - TrivialValue& operator=(TrivialValue&&) = default; - - absl::Nonnull get() { - return std::launder(reinterpret_cast(&value_[0])); - } - - absl::Nonnull get() const { - return std::launder(reinterpret_cast(&value_[0])); - } - - Value& operator*() ABSL_ATTRIBUTE_LIFETIME_BOUND { return *get(); } - - const Value& operator*() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - return *get(); - } - - absl::Nonnull operator->() { return get(); } - - absl::Nonnull operator->() const { return get(); } - - absl::string_view ToString() const; - - absl::string_view ToBytes() const; - - private: - friend TrivialValue MakeTrivialValue(const Value& value, - absl::Nonnull arena); - - explicit TrivialValue(const Value& other) { - std::memcpy(&value_[0], static_cast(std::addressof(other)), - sizeof(Value)); - } - - alignas(Value) char value_[sizeof(Value)]; -}; - } // namespace common_internal } // namespace cel diff --git a/common/values/bytes_value.h b/common/values/bytes_value.h index 913ccec59..26027fc71 100644 --- a/common/values/bytes_value.h +++ b/common/values/bytes_value.h @@ -31,6 +31,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "common/allocator.h" +#include "common/arena.h" #include "common/internal/byte_string.h" #include "common/memory.h" #include "common/type.h" @@ -49,9 +50,7 @@ class BytesValueInputStream; class BytesValueOutputStream; namespace common_internal { -class TrivialValue; - -absl::string_view LegacyBytesValue(const BytesValue& value, +absl::string_view LegacyBytesValue(const BytesValue& value, bool stable, absl::Nonnull arena); } // namespace common_internal @@ -199,12 +198,13 @@ class BytesValue final : private common_internal::ValueMixin { } private: - friend class common_internal::TrivialValue; friend class common_internal::ValueMixin; friend class BytesValueInputStream; friend class BytesValueOutputStream; friend absl::string_view common_internal::LegacyBytesValue( - const BytesValue& value, absl::Nonnull arena); + const BytesValue& value, bool stable, + absl::Nonnull arena); + friend struct ArenaTraits; explicit BytesValue(common_internal::ByteString value) noexcept : value_(std::move(value)) {} @@ -236,13 +236,22 @@ inline bool operator!=(absl::string_view lhs, const BytesValue& rhs) { namespace common_internal { -inline absl::string_view LegacyBytesValue(const BytesValue& value, +inline absl::string_view LegacyBytesValue(const BytesValue& value, bool stable, absl::Nonnull arena) { - return LegacyByteString(value.value_, arena); + return LegacyByteString(value.value_, stable, arena); } } // namespace common_internal +template <> +struct ArenaTraits { + using constructible = std::true_type; + + static bool trivially_destructible(const BytesValue& value) { + return ArenaTraits<>::trivially_destructible(value.value_); + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_BYTES_VALUE_H_ diff --git a/common/values/custom_list_value.cc b/common/values/custom_list_value.cc index ca4b24ace..eab029632 100644 --- a/common/values/custom_list_value.cc +++ b/common/values/custom_list_value.cc @@ -258,8 +258,7 @@ absl::Status CustomListValueInterface::Contains( } CustomListValue::CustomListValue() - : CustomListValue( - common_internal::MakeShared(&EmptyListValue::Get(), nullptr)) {} + : CustomListValue(Owned(Owner::None(), &EmptyListValue::Get())) {} CustomListValue CustomListValue::Clone( absl::Nonnull arena) const { @@ -269,7 +268,10 @@ CustomListValue CustomListValue::Clone( if (ABSL_PREDICT_FALSE(!interface_)) { return CustomListValue(); } - return interface_->Clone(arena); + if (interface_.arena() != arena) { + return interface_->Clone(arena); + } + return *this; } } // namespace cel diff --git a/common/values/custom_list_value.h b/common/values/custom_list_value.h index 4bbb6535c..91037c76e 100644 --- a/common/values/custom_list_value.h +++ b/common/values/custom_list_value.h @@ -37,6 +37,7 @@ #include "absl/status/statusor.h" #include "absl/strings/cord.h" #include "absl/strings/string_view.h" +#include "common/arena.h" #include "common/memory.h" #include "common/native_type.h" #include "common/value_kind.h" @@ -131,7 +132,7 @@ class CustomListValue static constexpr ValueKind kKind = CustomListValueInterface::kKind; // NOLINTNEXTLINE(google-explicit-constructor) - CustomListValue(Shared interface) + CustomListValue(Owned interface) : interface_(std::move(interface)) {} // By default, this creates an empty list whose type is `list(dyn)`. Unless @@ -279,8 +280,9 @@ class CustomListValue friend bool Is(const CustomListValue& lhs, const CustomListValue& rhs); friend class common_internal::ValueMixin; friend class common_internal::ListValueMixin; + friend struct ArenaTraits; - Shared interface_; + Owned interface_; }; inline void swap(CustomListValue& lhs, CustomListValue& rhs) noexcept { @@ -297,10 +299,6 @@ struct NativeTypeTraits final { static NativeTypeId Id(const CustomListValue& type) { return NativeTypeId::Of(*type.interface_); } - - static bool SkipDestructor(const CustomListValue& type) { - return NativeType::SkipDestructor(type.interface_); - } }; template @@ -311,16 +309,19 @@ struct NativeTypeTraits::Id(type); } - - static bool SkipDestructor(const T& type) { - return NativeTypeTraits::SkipDestructor(type); - } }; inline bool Is(const CustomListValue& lhs, const CustomListValue& rhs) { return lhs.interface_.operator->() == rhs.interface_.operator->(); } +template <> +struct ArenaTraits { + static bool trivially_destructible(const CustomListValue& value) { + return ArenaTraits<>::trivially_destructible(value.interface_); + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_PARSED_LIST_VALUE_H_ diff --git a/common/values/custom_map_value.cc b/common/values/custom_map_value.cc index a8d7c135e..36e417a47 100644 --- a/common/values/custom_map_value.cc +++ b/common/values/custom_map_value.cc @@ -317,8 +317,7 @@ absl::Status CustomMapValueInterface::Equal( } CustomMapValue::CustomMapValue() - : CustomMapValue( - common_internal::MakeShared(&EmptyMapValue::Get(), nullptr)) {} + : CustomMapValue(Owned(Owner::None(), &EmptyMapValue::Get())) {} CustomMapValue CustomMapValue::Clone( absl::Nonnull arena) const { @@ -328,7 +327,10 @@ CustomMapValue CustomMapValue::Clone( if (ABSL_PREDICT_FALSE(!interface_)) { return CustomMapValue(); } - return interface_->Clone(arena); + if (interface_.arena() != arena) { + return interface_->Clone(arena); + } + return *this; } } // namespace cel diff --git a/common/values/custom_map_value.h b/common/values/custom_map_value.h index 5d7be289e..a165e838e 100644 --- a/common/values/custom_map_value.h +++ b/common/values/custom_map_value.h @@ -37,6 +37,7 @@ #include "absl/status/statusor.h" #include "absl/strings/cord.h" #include "absl/strings/string_view.h" +#include "common/arena.h" #include "common/memory.h" #include "common/native_type.h" #include "common/value_kind.h" @@ -157,7 +158,7 @@ class CustomMapValue : private common_internal::MapValueMixin { static constexpr ValueKind kKind = CustomMapValueInterface::kKind; // NOLINTNEXTLINE(google-explicit-constructor) - CustomMapValue(Shared interface) + CustomMapValue(Owned interface) : interface_(std::move(interface)) {} // By default, this creates an empty map whose type is `map(dyn, dyn)`. Unless @@ -344,8 +345,9 @@ class CustomMapValue : private common_internal::MapValueMixin { friend struct NativeTypeTraits; friend class common_internal::ValueMixin; friend class common_internal::MapValueMixin; + friend struct ArenaTraits; - Shared interface_; + Owned interface_; }; inline void swap(CustomMapValue& lhs, CustomMapValue& rhs) noexcept { @@ -361,10 +363,6 @@ struct NativeTypeTraits final { static NativeTypeId Id(const CustomMapValue& type) { return NativeTypeId::Of(*type.interface_); } - - static bool SkipDestructor(const CustomMapValue& type) { - return NativeType::SkipDestructor(type.interface_); - } }; template @@ -375,9 +373,12 @@ struct NativeTypeTraits::Id(type); } +}; - static bool SkipDestructor(const T& type) { - return NativeTypeTraits::SkipDestructor(type); +template <> +struct ArenaTraits { + static bool trivially_destructible(const CustomMapValue& value) { + return ArenaTraits<>::trivially_destructible(value.interface_); } }; diff --git a/common/values/custom_struct_value.cc b/common/values/custom_struct_value.cc index b08dce83c..b92e8c979 100644 --- a/common/values/custom_struct_value.cc +++ b/common/values/custom_struct_value.cc @@ -64,7 +64,10 @@ CustomStructValue CustomStructValue::Clone( if (ABSL_PREDICT_FALSE(!interface_)) { return CustomStructValue(); } - return interface_->Clone(arena); + if (interface_.arena() != arena) { + return interface_->Clone(arena); + } + return *this; } absl::Status CustomStructValueInterface::Qualify( diff --git a/common/values/custom_struct_value.h b/common/values/custom_struct_value.h index e462e6bf7..62bd8202a 100644 --- a/common/values/custom_struct_value.h +++ b/common/values/custom_struct_value.h @@ -33,6 +33,7 @@ #include "absl/strings/string_view.h" #include "absl/types/span.h" #include "base/attribute.h" +#include "common/arena.h" #include "common/memory.h" #include "common/native_type.h" #include "common/type.h" @@ -124,7 +125,7 @@ class CustomStructValue static constexpr ValueKind kKind = CustomStructValueInterface::kKind; // NOLINTNEXTLINE(google-explicit-constructor) - CustomStructValue(Shared interface) + CustomStructValue(Owned interface) : interface_(std::move(interface)) {} CustomStructValue() = default; @@ -285,8 +286,9 @@ class CustomStructValue friend struct NativeTypeTraits; friend class common_internal::ValueMixin; friend class common_internal::StructValueMixin; + friend struct ArenaTraits; - Shared interface_; + Owned interface_; }; inline void swap(CustomStructValue& lhs, CustomStructValue& rhs) noexcept { @@ -303,10 +305,6 @@ struct NativeTypeTraits final { static NativeTypeId Id(const CustomStructValue& type) { return NativeTypeId::Of(*type.interface_); } - - static bool SkipDestructor(const CustomStructValue& type) { - return NativeType::SkipDestructor(type.interface_); - } }; template @@ -318,9 +316,12 @@ struct NativeTypeTraits< static NativeTypeId Id(const T& type) { return NativeTypeTraits::Id(type); } +}; - static bool SkipDestructor(const T& type) { - return NativeTypeTraits::SkipDestructor(type); +template <> +struct ArenaTraits { + static bool trivially_destructible(const CustomStructValue& value) { + return ArenaTraits<>::trivially_destructible(value.interface_); } }; diff --git a/common/values/error_value.h b/common/values/error_value.h index 5dab1dfd7..b3959380b 100644 --- a/common/values/error_value.h +++ b/common/values/error_value.h @@ -31,6 +31,7 @@ #include "absl/status/status.h" #include "absl/strings/cord.h" #include "absl/strings/string_view.h" +#include "common/arena.h" #include "common/type.h" #include "common/value_kind.h" #include "common/values/values.h" @@ -124,6 +125,7 @@ class ABSL_ATTRIBUTE_TRIVIAL_ABI ErrorValue final private: friend class common_internal::ValueMixin; + friend struct ArenaTraits; ErrorValue(absl::Nonnull arena, absl::Nonnull status) @@ -215,6 +217,13 @@ class ErrorValueReturn final { } }; +template <> +struct ArenaTraits { + static bool trivially_destructible(const ErrorValue& value) { + return value.arena_ != nullptr; + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_ERROR_VALUE_H_ diff --git a/common/values/list_value.h b/common/values/list_value.h index 9d56f94f6..fec941440 100644 --- a/common/values/list_value.h +++ b/common/values/list_value.h @@ -40,6 +40,7 @@ #include "absl/types/optional.h" #include "absl/types/variant.h" #include "absl/utility/utility.h" +#include "common/arena.h" #include "common/native_type.h" #include "common/optional_ref.h" #include "common/value_kind.h" @@ -286,6 +287,7 @@ class ListValue final : private common_internal::ListValueMixin { friend struct NativeTypeTraits; friend class common_internal::ValueMixin; friend class common_internal::ListValueMixin; + friend struct ArenaTraits; common_internal::ValueVariant ToValueVariant() const&; common_internal::ValueVariant ToValueVariant() &&; @@ -312,11 +314,14 @@ struct NativeTypeTraits final { }, value.variant_); } +}; - static bool SkipDestructor(const ListValue& value) { +template <> +struct ArenaTraits { + static bool trivially_destructible(const ListValue& value) { return absl::visit( [](const auto& alternative) -> bool { - return NativeType::SkipDestructor(alternative); + return ArenaTraits<>::trivially_destructible(alternative); }, value.variant_); } @@ -328,6 +333,8 @@ class ListValueBuilder { virtual absl::Status Add(Value value) = 0; + virtual void UnsafeAdd(Value value) = 0; + virtual bool IsEmpty() const { return Size() == 0; } virtual size_t Size() const = 0; diff --git a/common/values/list_value_builder.h b/common/values/list_value_builder.h index 0c631adc9..cb7585d29 100644 --- a/common/values/list_value_builder.h +++ b/common/values/list_value_builder.h @@ -21,7 +21,6 @@ #include "absl/base/nullability.h" #include "absl/status/status.h" #include "absl/status/statusor.h" -#include "common/allocator.h" #include "common/memory.h" #include "common/native_type.h" #include "common/value.h" @@ -94,7 +93,8 @@ class MutableCompatListValue : public MutableListValue, } }; -Shared NewMutableListValue(Allocator<> allocator); +Owned NewMutableListValue( + absl::Nonnull arena); bool IsMutableListValue(const Value& value); bool IsMutableListValue(const ListValue& value); @@ -110,7 +110,7 @@ const MutableListValue& GetMutableListValue( const ListValue& value ABSL_ATTRIBUTE_LIFETIME_BOUND); absl::Nonnull NewListValueBuilder( - Allocator<> allocator); + absl::Nonnull arena); } // namespace common_internal diff --git a/common/values/map_value.h b/common/values/map_value.h index a0d70441b..fe03532f1 100644 --- a/common/values/map_value.h +++ b/common/values/map_value.h @@ -41,6 +41,7 @@ #include "absl/types/optional.h" #include "absl/types/variant.h" #include "absl/utility/utility.h" +#include "common/arena.h" #include "common/native_type.h" #include "common/optional_ref.h" #include "common/value_kind.h" @@ -307,6 +308,7 @@ class MapValue final : private common_internal::MapValueMixin { friend struct NativeTypeTraits; friend class common_internal::ValueMixin; friend class common_internal::MapValueMixin; + friend struct ArenaTraits; common_internal::ValueVariant ToValueVariant() const&; common_internal::ValueVariant ToValueVariant() &&; @@ -333,11 +335,14 @@ struct NativeTypeTraits final { }, value.variant_); } +}; - static bool SkipDestructor(const MapValue& value) { +template <> +struct ArenaTraits { + static bool trivially_destructible(const MapValue& value) { return absl::visit( [](const auto& alternative) -> bool { - return NativeType::SkipDestructor(alternative); + return ArenaTraits<>::trivially_destructible(alternative); }, value.variant_); } @@ -349,6 +354,8 @@ class MapValueBuilder { virtual absl::Status Put(Value key, Value value) = 0; + virtual void UnsafePut(Value key, Value value) = 0; + virtual bool IsEmpty() const { return Size() == 0; } virtual size_t Size() const = 0; diff --git a/common/values/map_value_builder.h b/common/values/map_value_builder.h index 89fcb15e4..ae8344eaa 100644 --- a/common/values/map_value_builder.h +++ b/common/values/map_value_builder.h @@ -21,7 +21,6 @@ #include "absl/base/nullability.h" #include "absl/status/status.h" #include "absl/status/statusor.h" -#include "common/allocator.h" #include "common/memory.h" #include "common/native_type.h" #include "common/value.h" @@ -100,7 +99,7 @@ class MutableCompatMapValue : public MutableMapValue, } }; -Shared NewMutableMapValue(Allocator<> allocator); +Owned NewMutableMapValue(absl::Nonnull arena); bool IsMutableMapValue(const Value& value); bool IsMutableMapValue(const MapValue& value); @@ -116,9 +115,7 @@ const MutableMapValue& GetMutableMapValue( const MapValue& value ABSL_ATTRIBUTE_LIFETIME_BOUND); absl::Nonnull NewMapValueBuilder( - Allocator<> allocator); -absl::Nonnull NewMapValueBuilder( - ValueFactory& value_factory); + absl::Nonnull arena); } // namespace common_internal diff --git a/common/values/message_value.h b/common/values/message_value.h index faaa8b7ea..5e3770bfe 100644 --- a/common/values/message_value.h +++ b/common/values/message_value.h @@ -40,6 +40,7 @@ #include "absl/types/variant.h" #include "absl/utility/utility.h" #include "base/attribute.h" +#include "common/arena.h" #include "common/optional_ref.h" #include "common/type.h" #include "common/value_kind.h" @@ -236,6 +237,7 @@ class MessageValue final friend class StructValue; friend class common_internal::ValueMixin; friend class common_internal::StructValueMixin; + friend struct ArenaTraits; common_internal::ValueVariant ToValueVariant() const&; common_internal::ValueVariant ToValueVariant() &&; @@ -250,6 +252,17 @@ inline std::ostream& operator<<(std::ostream& out, const MessageValue& value) { return out << value.DebugString(); } +template <> +struct ArenaTraits { + static bool trivially_destructible(const MessageValue& value) { + return absl::visit( + [](const auto& alternative) -> bool { + return ArenaTraits<>::trivially_destructible(alternative); + }, + value.variant_); + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_MESSAGE_VALUE_H_ diff --git a/common/values/opaque_value.cc b/common/values/opaque_value.cc index 17e8d964b..7688f0af8 100644 --- a/common/values/opaque_value.cc +++ b/common/values/opaque_value.cc @@ -39,7 +39,10 @@ OpaqueValue OpaqueValue::Clone(absl::Nonnull arena) co if (ABSL_PREDICT_FALSE(!interface_)) { return OpaqueValue(); } - return interface_->Clone(arena); + if (interface_.arena() != arena) { + return interface_->Clone(arena); + } + return *this; } bool OpaqueValue::IsOptional() const { diff --git a/common/values/opaque_value.h b/common/values/opaque_value.h index 4601ce2fc..cff391a26 100644 --- a/common/values/opaque_value.h +++ b/common/values/opaque_value.h @@ -36,6 +36,7 @@ #include "absl/strings/cord.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "common/arena.h" #include "common/memory.h" #include "common/native_type.h" #include "common/optional_ref.h" @@ -84,7 +85,7 @@ class OpaqueValue : private common_internal::OpaqueValueMixin { template >>> // NOLINTNEXTLINE(google-explicit-constructor) - OpaqueValue(Shared interface) : interface_(std::move(interface)) {} + OpaqueValue(Owned interface) : interface_(std::move(interface)) {} OpaqueValue() = default; OpaqueValue(const OpaqueValue&) = default; @@ -213,8 +214,9 @@ class OpaqueValue : private common_internal::OpaqueValueMixin { friend struct NativeTypeTraits; friend class common_internal::ValueMixin; friend class common_internal::OpaqueValueMixin; + friend struct ArenaTraits; - Shared interface_; + Owned interface_; }; inline void swap(OpaqueValue& lhs, OpaqueValue& rhs) noexcept { lhs.swap(rhs); } @@ -228,10 +230,6 @@ struct NativeTypeTraits final { static NativeTypeId Id(const OpaqueValue& type) { return NativeTypeId::Of(*type.interface_); } - - static bool SkipDestructor(const OpaqueValue& type) { - return NativeType::SkipDestructor(*type.interface_); - } }; template @@ -242,9 +240,12 @@ struct NativeTypeTraits::Id(type); } +}; - static bool SkipDestructor(const T& type) { - return NativeTypeTraits::SkipDestructor(type); +template <> +struct ArenaTraits { + static bool trivially_destructible(const OpaqueValue& value) { + return ArenaTraits<>::trivially_destructible(value.interface_); } }; diff --git a/common/values/optional_value.cc b/common/values/optional_value.cc index 916960f7a..e55e7a430 100644 --- a/common/values/optional_value.cc +++ b/common/values/optional_value.cc @@ -21,6 +21,7 @@ #include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "common/allocator.h" +#include "common/arena.h" #include "common/memory.h" #include "common/native_type.h" #include "common/value.h" @@ -53,10 +54,7 @@ class FullOptionalValue final : public OptionalValueInterface { public: explicit FullOptionalValue(cel::Value value) : value_(std::move(value)) {} - OpaqueValue Clone(absl::Nonnull arena) const override { - return MemoryManager::Pooling(arena).MakeShared( - value_.Clone(arena)); - } + OpaqueValue Clone(absl::Nonnull arena) const override; bool HasValue() const override { return true; } @@ -66,12 +64,30 @@ class FullOptionalValue final : public OptionalValueInterface { private: friend struct NativeTypeTraits; + friend struct ArenaTraits; const cel::Value value_; }; } // namespace +template <> +struct ArenaTraits { + static bool trivially_destructible(const FullOptionalValue& value) { + return ArenaTraits<>::trivially_destructible(value.value_); + } +}; + +namespace { + +OpaqueValue FullOptionalValue::Clone( + absl::Nonnull arena) const { + return OptionalValue( + AllocateShared(arena, value_.Clone(arena))); +} + +} // namespace + template <> struct NativeTypeTraits { static bool SkipDestructor(const FullOptionalValue& value) { @@ -92,13 +108,12 @@ OptionalValue OptionalValue::Of(cel::Value value, Allocator<> allocator) { ABSL_DCHECK(value.kind() != ValueKind::kError && value.kind() != ValueKind::kUnknown); return OptionalValue( - MemoryManagerRef(allocator).MakeShared( - std::move(value))); + AllocateShared(allocator, std::move(value))); } OptionalValue OptionalValue::None() { static const absl::NoDestructor empty; - return OptionalValue(common_internal::MakeShared(&*empty, nullptr)); + return OptionalValue(Owned(Owner::None(), &*empty)); } absl::Status OptionalValueInterface::Equal( diff --git a/common/values/optional_value.h b/common/values/optional_value.h index e346c521a..571a62d75 100644 --- a/common/values/optional_value.h +++ b/common/values/optional_value.h @@ -30,6 +30,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "common/allocator.h" +#include "common/arena.h" #include "common/memory.h" #include "common/native_type.h" #include "common/optional_ref.h" @@ -95,7 +96,7 @@ class OptionalValue final : public OpaqueValue { template >>> // NOLINTNEXTLINE(google-explicit-constructor) - OptionalValue(Shared interface) : OpaqueValue(std::move(interface)) {} + OptionalValue(Owned interface) : OpaqueValue(std::move(interface)) {} OptionalType GetRuntimeType() const { return (*this)->GetRuntimeType().GetOptional(); @@ -160,6 +161,9 @@ class OptionalValue final : public OpaqueValue { std::enable_if_t, absl::optional> Get() const&& = delete; + + private: + friend struct ArenaTraits; }; inline optional_ref OpaqueValue::AsOptional() & @@ -232,6 +236,13 @@ OpaqueValue::Get() const&& { return std::move(*this).GetOptional(); } +template <> +struct ArenaTraits { + static bool trivially_destructible(const OptionalValue& value) { + return ArenaTraits::trivially_destructible(value); + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_OPTIONAL_VALUE_H_ diff --git a/common/values/parsed_json_list_value.h b/common/values/parsed_json_list_value.h index 6e1a63fc2..fa435a660 100644 --- a/common/values/parsed_json_list_value.h +++ b/common/values/parsed_json_list_value.h @@ -33,6 +33,7 @@ #include "absl/status/statusor.h" #include "absl/strings/cord.h" #include "absl/strings/string_view.h" +#include "common/arena.h" #include "common/memory.h" #include "common/type.h" #include "common/value_kind.h" @@ -171,6 +172,7 @@ class ParsedJsonListValue final friend class ParsedRepeatedFieldValue; friend class common_internal::ValueMixin; friend class common_internal::ListValueMixin; + friend struct ArenaTraits; static absl::Status CheckListValue( absl::Nullable message) { @@ -192,6 +194,13 @@ inline std::ostream& operator<<(std::ostream& out, return out << value.DebugString(); } +template <> +struct ArenaTraits { + static bool trivially_destructible(const ParsedJsonListValue& value) { + return ArenaTraits<>::trivially_destructible(value.value_); + } +}; + } // namespace cel namespace std { diff --git a/common/values/parsed_json_map_value.h b/common/values/parsed_json_map_value.h index 89f0760ed..793f53066 100644 --- a/common/values/parsed_json_map_value.h +++ b/common/values/parsed_json_map_value.h @@ -33,6 +33,7 @@ #include "absl/status/statusor.h" #include "absl/strings/cord.h" #include "absl/strings/string_view.h" +#include "common/arena.h" #include "common/memory.h" #include "common/type.h" #include "common/value_kind.h" @@ -193,6 +194,7 @@ class ParsedJsonMapValue final friend class ParsedMapFieldValue; friend class common_internal::ValueMixin; friend class common_internal::MapValueMixin; + friend struct ArenaTraits; static absl::Status CheckStruct( absl::Nullable message) { @@ -214,6 +216,13 @@ inline std::ostream& operator<<(std::ostream& out, return out << value.DebugString(); } +template <> +struct ArenaTraits { + static bool trivially_destructible(const ParsedJsonMapValue& value) { + return ArenaTraits<>::trivially_destructible(value.value_); + } +}; + } // namespace cel namespace std { diff --git a/common/values/parsed_map_field_value.h b/common/values/parsed_map_field_value.h index 02f907582..5200a64b3 100644 --- a/common/values/parsed_map_field_value.h +++ b/common/values/parsed_map_field_value.h @@ -32,6 +32,7 @@ #include "absl/status/statusor.h" #include "absl/strings/cord.h" #include "absl/strings/string_view.h" +#include "common/arena.h" #include "common/memory.h" #include "common/type.h" #include "common/value_kind.h" @@ -188,6 +189,7 @@ class ParsedMapFieldValue final friend class ParsedJsonMapValue; friend class common_internal::ValueMixin; friend class common_internal::MapValueMixin; + friend struct ArenaTraits; absl::Nonnull GetReflection() const; @@ -200,6 +202,13 @@ inline std::ostream& operator<<(std::ostream& out, return out << value.DebugString(); } +template <> +struct ArenaTraits { + static bool trivially_destructible(const ParsedMapFieldValue& value) { + return ArenaTraits<>::trivially_destructible(value.message_); + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_PARSED_MAP_FIELD_VALUE_H_ diff --git a/common/values/parsed_message_value.h b/common/values/parsed_message_value.h index d1d77d8ae..1f358f3d9 100644 --- a/common/values/parsed_message_value.h +++ b/common/values/parsed_message_value.h @@ -37,6 +37,7 @@ #include "absl/types/span.h" #include "base/attribute.h" #include "common/allocator.h" +#include "common/arena.h" #include "common/memory.h" #include "common/type.h" #include "common/value_kind.h" @@ -182,6 +183,7 @@ class ParsedMessageValue final friend class StructValue; friend class common_internal::ValueMixin; friend class common_internal::StructValueMixin; + friend struct ArenaTraits; absl::Status GetField( absl::Nonnull field, @@ -200,6 +202,13 @@ inline std::ostream& operator<<(std::ostream& out, return out << value.DebugString(); } +template <> +struct ArenaTraits { + static bool trivially_destructible(const ParsedMessageValue& value) { + return ArenaTraits<>::trivially_destructible(value.value_); + } +}; + } // namespace cel namespace std { diff --git a/common/values/parsed_repeated_field_value.h b/common/values/parsed_repeated_field_value.h index 1b43686d8..e7ba4ef39 100644 --- a/common/values/parsed_repeated_field_value.h +++ b/common/values/parsed_repeated_field_value.h @@ -31,6 +31,7 @@ #include "absl/status/statusor.h" #include "absl/strings/cord.h" #include "absl/strings/string_view.h" +#include "common/arena.h" #include "common/memory.h" #include "common/type.h" #include "common/value_kind.h" @@ -165,6 +166,7 @@ class ParsedRepeatedFieldValue final friend class ParsedJsonListValue; friend class common_internal::ValueMixin; friend class common_internal::ListValueMixin; + friend struct ArenaTraits; absl::Nonnull GetReflection() const; @@ -177,6 +179,13 @@ inline std::ostream& operator<<(std::ostream& out, return out << value.DebugString(); } +template <> +struct ArenaTraits { + static bool trivially_destructible(const ParsedRepeatedFieldValue& value) { + return ArenaTraits<>::trivially_destructible(value.message_); + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_PARSED_REPEATED_FIELD_VALUE_H_ diff --git a/common/values/string_value.h b/common/values/string_value.h index 5c170644f..ca96c1328 100644 --- a/common/values/string_value.h +++ b/common/values/string_value.h @@ -31,6 +31,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "common/allocator.h" +#include "common/arena.h" #include "common/internal/byte_string.h" #include "common/memory.h" #include "common/type.h" @@ -47,9 +48,7 @@ class StringValue; class TypeManager; namespace common_internal { -class TrivialValue; - -absl::string_view LegacyStringValue(const StringValue& value, +absl::string_view LegacyStringValue(const StringValue& value, bool stable, absl::Nonnull arena); } // namespace common_internal @@ -209,10 +208,11 @@ class StringValue final : private common_internal::ValueMixin { } private: - friend class common_internal::TrivialValue; friend class common_internal::ValueMixin; friend absl::string_view common_internal::LegacyStringValue( - const StringValue& value, absl::Nonnull arena); + const StringValue& value, bool stable, + absl::Nonnull arena); + friend struct ArenaTraits; explicit StringValue(common_internal::ByteString value) noexcept : value_(std::move(value)) {} @@ -281,12 +281,22 @@ inline std::ostream& operator<<(std::ostream& out, const StringValue& value) { namespace common_internal { inline absl::string_view LegacyStringValue( - const StringValue& value, absl::Nonnull arena) { - return LegacyByteString(value.value_, arena); + const StringValue& value, bool stable, + absl::Nonnull arena) { + return LegacyByteString(value.value_, stable, arena); } } // namespace common_internal +template <> +struct ArenaTraits { + using constructible = std::true_type; + + static bool trivially_destructible(const StringValue& value) { + return ArenaTraits<>::trivially_destructible(value.value_); + } +}; + } // namespace cel #endif // THIRD_PARTY_CEL_CPP_COMMON_VALUES_STRING_VALUE_H_ diff --git a/common/values/struct_value.h b/common/values/struct_value.h index a8edfe741..f1bfb9872 100644 --- a/common/values/struct_value.h +++ b/common/values/struct_value.h @@ -41,6 +41,7 @@ #include "absl/types/variant.h" #include "absl/utility/utility.h" #include "base/attribute.h" +#include "common/arena.h" #include "common/native_type.h" #include "common/optional_ref.h" #include "common/type.h" @@ -363,6 +364,7 @@ class StructValue final friend struct NativeTypeTraits; friend class common_internal::ValueMixin; friend class common_internal::StructValueMixin; + friend struct ArenaTraits; common_internal::ValueVariant ToValueVariant() const&; common_internal::ValueVariant ToValueVariant() &&; @@ -463,20 +465,15 @@ struct NativeTypeTraits final { }, value.variant_); } +}; - static bool SkipDestructor(const StructValue& value) { +template <> +struct ArenaTraits { + static bool trivially_destructible(const StructValue& value) { value.AssertIsValid(); return absl::visit( [](const auto& alternative) -> bool { - if constexpr (std::is_same_v< - absl::remove_cvref_t, - absl::monostate>) { - // In optimized builds, we just say we should skip the destructor. - // In debug builds we cannot reach here. - return true; - } else { - return NativeType::SkipDestructor(alternative); - } + return ArenaTraits<>::trivially_destructible(alternative); }, value.variant_); } diff --git a/common/values/value_builder.cc b/common/values/value_builder.cc index 834280f83..d6cf4ed6e 100644 --- a/common/values/value_builder.cc +++ b/common/values/value_builder.cc @@ -13,14 +13,17 @@ // limitations under the License. #include +#include #include #include #include #include +#include #include #include #include "absl/base/call_once.h" +#include "absl/base/casts.h" #include "absl/base/nullability.h" #include "absl/base/optimization.h" #include "absl/container/flat_hash_map.h" @@ -33,7 +36,7 @@ #include "absl/types/optional.h" #include "absl/types/span.h" #include "common/allocator.h" -#include "common/internal/reference_count.h" +#include "common/arena.h" #include "common/legacy_value.h" #include "common/memory.h" #include "common/native_type.h" @@ -44,6 +47,7 @@ #include "common/values/map_value_builder.h" #include "eval/public/cel_value.h" #include "internal/casts.h" +#include "internal/manual.h" #include "internal/status_macros.h" #include "internal/well_known_types.h" #include "google/protobuf/arena.h" @@ -61,14 +65,11 @@ using ::cel::well_known_types::StructReflection; using ::cel::well_known_types::ValueReflection; using ::google::api::expr::runtime::CelValue; -using TrivialValueVector = - std::vector>; -using NonTrivialValueVector = - std::vector>; +using ValueVector = std::vector>; absl::Status CheckListElement(const Value& value) { if (auto error_value = value.AsError(); ABSL_PREDICT_FALSE(error_value)) { - return error_value->NativeValue(); + return error_value->ToStatus(); } if (auto unknown_value = value.AsUnknown(); ABSL_PREDICT_FALSE(unknown_value)) { @@ -123,10 +124,9 @@ absl::Status ListValueToJson( reflection.MutableListValue(json)); } -template -class ListValueImplIterator final : public ValueIterator { +class CompatListValueImplIterator final : public ValueIterator { public: - explicit ListValueImplIterator(absl::Span elements) + explicit CompatListValueImplIterator(absl::Span elements) : elements_(elements) {} bool HasNext() override { return index_ < elements_.size(); } @@ -141,48 +141,78 @@ class ListValueImplIterator final : public ValueIterator { "ValueManager::Next called after ValueManager::HasNext returned " "false"); } - *result = *elements_[index_++]; + *result = elements_[index_++]; return absl::OkStatus(); } private: - const absl::Span elements_; + const absl::Span elements_; size_t index_ = 0; }; struct ValueFormatter { - void operator()( - std::string* out, - const std::pair& value) const { - (*this)(out, *value.first); + void operator()(std::string* out, + const std::pair& value) const { + (*this)(out, value.first); out->append(": "); - (*this)(out, *value.second); + (*this)(out, value.second); } - void operator()( - std::string* out, - const std::pair& value) const { - (*this)(out, *value.first); - out->append(": "); - (*this)(out, *value.second); + void operator()(std::string* out, const Value& value) const { + out->append(value.DebugString()); } +}; - void operator()(std::string* out, const TrivialValue& value) const { - (*this)(out, *value); +class ListValueBuilderImpl final : public ListValueBuilder { + public: + explicit ListValueBuilderImpl(absl::Nonnull arena) + : arena_(arena) { + elements_.Construct(arena); } - void operator()(std::string* out, const NonTrivialValue& value) const { - (*this)(out, *value); + ~ListValueBuilderImpl() override { + if (!elements_trivially_destructible_) { + elements_.Destruct(); + } } - void operator()(std::string* out, const Value& value) const { - out->append(value.DebugString()); + absl::Status Add(Value value) override { + CEL_RETURN_IF_ERROR(CheckListElement(value)); + UnsafeAdd(std::move(value)); + return absl::OkStatus(); + } + + void UnsafeAdd(Value value) override { + ABSL_DCHECK_OK(CheckListElement(value)); + elements_->emplace_back(std::move(value)); + if (elements_trivially_destructible_) { + elements_trivially_destructible_ = + ArenaTraits<>::trivially_destructible(elements_->back()); + } } + + size_t Size() const override { return elements_->size(); } + + void Reserve(size_t capacity) override { elements_->reserve(capacity); } + + ListValue Build() && override; + + CustomListValue BuildCustom() &&; + + absl::Nonnull BuildCompat() &&; + + absl::Nonnull BuildCompatAt( + absl::Nonnull address) &&; + + private: + absl::Nonnull const arena_; + internal::Manual elements_; + bool elements_trivially_destructible_ = true; }; -class TrivialListValueImpl final : public CompatListValue { +class CompatListValueImpl final : public CompatListValue { public: - explicit TrivialListValueImpl(TrivialValueVector&& elements) + explicit CompatListValueImpl(ValueVector&& elements) : elements_(std::move(elements)) {} std::string DebugString() const override { @@ -206,13 +236,14 @@ class TrivialListValueImpl final : public CompatListValue { } CustomListValue Clone(absl::Nonnull arena) const override { - // This is unreachable with the current logic in CustomListValue, but could - // be called once we keep track of the owning arena in CustomListValue. - TrivialValueVector cloned_elements(elements_, - ArenaAllocator{arena}); - return CustomListValue( - MemoryManager::Pooling(arena).MakeShared( - std::move(cloned_elements))); + ABSL_DCHECK(arena != nullptr); + + ListValueBuilderImpl builder(arena); + builder.Reserve(elements_.size()); + for (const auto& element : elements_) { + builder.UnsafeAdd(element.Clone(arena)); + } + return std::move(builder).BuildCustom(); } size_t Size() const override { return elements_.size(); } @@ -224,7 +255,7 @@ class TrivialListValueImpl final : public CompatListValue { absl::Nonnull arena) const override { const size_t size = elements_.size(); for (size_t i = 0; i < size; ++i) { - CEL_ASSIGN_OR_RETURN(auto ok, callback(i, *elements_[i])); + CEL_ASSIGN_OR_RETURN(auto ok, callback(i, elements_[i])); if (!ok) { break; } @@ -233,7 +264,7 @@ class TrivialListValueImpl final : public CompatListValue { } absl::StatusOr> NewIterator() const override { - return std::make_unique>( + return std::make_unique( absl::MakeConstSpan(elements_)); } @@ -250,11 +281,12 @@ class TrivialListValueImpl final : public CompatListValue { } if (ABSL_PREDICT_FALSE(index < 0 || index >= size())) { return CelValue::CreateError(google::protobuf::Arena::Create( - arena, IndexOutOfBoundsError(index).NativeValue())); + arena, IndexOutOfBoundsError(index).ToStatus())); } - return common_internal::LegacyTrivialValue( - arena != nullptr ? arena : elements_.get_allocator().arena(), - elements_[index]); + return common_internal::UnsafeLegacyValue( + elements_[index], + /*stable=*/true, + arena != nullptr ? arena : elements_.get_allocator().arena()); } int size() const override { return static_cast(Size()); } @@ -266,12 +298,12 @@ class TrivialListValueImpl final : public CompatListValue { absl::Nonnull message_factory, absl::Nonnull arena, absl::Nonnull result) const override { - *result = *elements_[index]; + *result = elements_[index]; return absl::OkStatus(); } private: - const TrivialValueVector elements_; + const ValueVector elements_; }; } // namespace @@ -279,97 +311,52 @@ class TrivialListValueImpl final : public CompatListValue { } // namespace common_internal template <> -struct NativeTypeTraits { - static bool SkipDestructor(const common_internal::TrivialListValueImpl&) { - return true; - } +struct ArenaTraits { + using always_trivially_destructible = std::true_type; }; namespace common_internal { namespace { -class NonTrivialListValueImpl final : public CustomListValueInterface { - public: - explicit NonTrivialListValueImpl(NonTrivialValueVector&& elements) - : elements_(std::move(elements)) {} - - std::string DebugString() const override { - return absl::StrCat("[", absl::StrJoin(elements_, ", ", ValueFormatter{}), - "]"); - } - - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return ListValueToJson(elements_, descriptor_pool, message_factory, json); - } - - absl::Status ConvertToJsonArray( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return ListValueToJsonArray(elements_, descriptor_pool, message_factory, - json); - } - - CustomListValue Clone(absl::Nonnull arena) const override { - TrivialValueVector cloned_elements(ArenaAllocator{arena}); - cloned_elements.reserve(elements_.size()); - for (const auto& element : elements_) { - cloned_elements.emplace_back(MakeTrivialValue(*element, arena)); - } - return CustomListValue( - MemoryManager::Pooling(arena).MakeShared( - std::move(cloned_elements))); - } - - size_t Size() const override { return elements_.size(); } - - absl::Status ForEach( - ForEachWithIndexCallback callback, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena) const override { - const size_t size = elements_.size(); - for (size_t i = 0; i < size; ++i) { - CEL_ASSIGN_OR_RETURN(auto ok, callback(i, *elements_[i])); - if (!ok) { - break; - } - } - return absl::OkStatus(); +ListValue ListValueBuilderImpl::Build() && { + if (elements_->empty()) { + return ListValue(); } + return std::move(*this).BuildCustom(); +} - absl::StatusOr> NewIterator() const override { - return std::make_unique>( - absl::MakeConstSpan(elements_)); +CustomListValue ListValueBuilderImpl::BuildCustom() && { + if (elements_->empty()) { + return CustomListValue(Owned(Owner::Arena(arena_), EmptyCompatListValue())); } + return CustomListValue( + Owned(Owner::Arena(arena_), std::move(*this).BuildCompat())); +} - protected: - absl::Status GetImpl( - size_t index, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { - *result = *elements_[index]; - return absl::OkStatus(); +absl::Nonnull ListValueBuilderImpl::BuildCompat() && { + if (elements_->empty()) { + return EmptyCompatListValue(); } + return std::move(*this).BuildCompatAt(arena_->AllocateAligned( + sizeof(CompatListValueImpl), alignof(CompatListValueImpl))); +} - private: - NativeTypeId GetNativeTypeId() const override { - return NativeTypeId::For(); +absl::Nonnull ListValueBuilderImpl::BuildCompatAt( + absl::Nonnull address) && { + absl::Nonnull impl = + ::new (address) CompatListValueImpl(std::move(*elements_)); + if (!elements_trivially_destructible_) { + arena_->OwnDestructor(impl); + elements_trivially_destructible_ = true; } + return impl; +} - const NonTrivialValueVector elements_; -}; - -class TrivialMutableListValueImpl final : public MutableCompatListValue { +class MutableCompatListValueImpl final : public MutableCompatListValue { public: - explicit TrivialMutableListValueImpl(absl::Nonnull arena) - : elements_(ArenaAllocator{arena}) {} + explicit MutableCompatListValueImpl(absl::Nonnull arena) + : elements_(arena) {} std::string DebugString() const override { return absl::StrCat("[", absl::StrJoin(elements_, ", ", ValueFormatter{}), @@ -392,13 +379,14 @@ class TrivialMutableListValueImpl final : public MutableCompatListValue { } CustomListValue Clone(absl::Nonnull arena) const override { - // This is unreachable with the current logic in CustomListValue, but could - // be called once we keep track of the owning arena in CustomListValue. - TrivialValueVector cloned_elements(elements_, - ArenaAllocator{arena}); - return CustomListValue( - MemoryManager::Pooling(arena).MakeShared( - std::move(cloned_elements))); + ABSL_DCHECK(arena != nullptr); + + ListValueBuilderImpl builder(arena); + builder.Reserve(elements_.size()); + for (const auto& element : elements_) { + builder.UnsafeAdd(element.Clone(arena)); + } + return std::move(builder).BuildCustom(); } size_t Size() const override { return elements_.size(); } @@ -410,7 +398,7 @@ class TrivialMutableListValueImpl final : public MutableCompatListValue { absl::Nonnull arena) const override { const size_t size = elements_.size(); for (size_t i = 0; i < size; ++i) { - CEL_ASSIGN_OR_RETURN(auto ok, callback(i, *elements_[i])); + CEL_ASSIGN_OR_RETURN(auto ok, callback(i, elements_[i])); if (!ok) { break; } @@ -419,7 +407,7 @@ class TrivialMutableListValueImpl final : public MutableCompatListValue { } absl::StatusOr> NewIterator() const override { - return std::make_unique>( + return std::make_unique( absl::MakeConstSpan(elements_)); } @@ -436,19 +424,26 @@ class TrivialMutableListValueImpl final : public MutableCompatListValue { } if (ABSL_PREDICT_FALSE(index < 0 || index >= size())) { return CelValue::CreateError(google::protobuf::Arena::Create( - arena, IndexOutOfBoundsError(index).NativeValue())); + arena, IndexOutOfBoundsError(index).ToStatus())); } - return common_internal::LegacyTrivialValue( - arena != nullptr ? arena : elements_.get_allocator().arena(), - elements_[index]); + return common_internal::UnsafeLegacyValue( + elements_[index], /*stable=*/false, + arena != nullptr ? arena : elements_.get_allocator().arena()); } int size() const override { return static_cast(Size()); } absl::Status Append(Value value) const override { CEL_RETURN_IF_ERROR(CheckListElement(value)); - elements_.emplace_back( - MakeTrivialValue(value, elements_.get_allocator().arena())); + elements_.emplace_back(std::move(value)); + if (elements_trivially_destructible_) { + elements_trivially_destructible_ = + ArenaTraits<>::trivially_destructible(elements_.back()); + if (!elements_trivially_destructible_) { + elements_.get_allocator().arena()->OwnDestructor( + const_cast(this)); + } + } return absl::OkStatus(); } @@ -461,12 +456,13 @@ class TrivialMutableListValueImpl final : public MutableCompatListValue { absl::Nonnull message_factory, absl::Nonnull arena, absl::Nonnull result) const override { - *result = *elements_[index]; + *result = elements_[index]; return absl::OkStatus(); } private: - mutable TrivialValueVector elements_; + mutable ValueVector elements_; + mutable bool elements_trivially_destructible_ = true; }; } // namespace @@ -474,184 +470,40 @@ class TrivialMutableListValueImpl final : public MutableCompatListValue { } // namespace common_internal template <> -struct NativeTypeTraits { - static bool SkipDestructor( - const common_internal::TrivialMutableListValueImpl&) { - return true; - } -}; - -namespace common_internal { - -namespace { - -class NonTrivialMutableListValueImpl final : public MutableListValue { - public: - NonTrivialMutableListValueImpl() = default; - - std::string DebugString() const override { - return absl::StrCat("[", absl::StrJoin(elements_, ", ", ValueFormatter{}), - "]"); - } - - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return ListValueToJson(elements_, descriptor_pool, message_factory, json); - } - - absl::Status ConvertToJsonArray( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return ListValueToJsonArray(elements_, descriptor_pool, message_factory, - json); - } - - CustomListValue Clone(absl::Nonnull arena) const override { - TrivialValueVector cloned_elements(ArenaAllocator{arena}); - cloned_elements.reserve(elements_.size()); - for (const auto& element : elements_) { - cloned_elements.emplace_back(MakeTrivialValue(*element, arena)); - } - return CustomListValue( - MemoryManager::Pooling(arena).MakeShared( - std::move(cloned_elements))); - } - - size_t Size() const override { return elements_.size(); } - - absl::Status ForEach( - ForEachWithIndexCallback callback, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena) const override { - const size_t size = elements_.size(); - for (size_t i = 0; i < size; ++i) { - CEL_ASSIGN_OR_RETURN(auto ok, callback(i, *elements_[i])); - if (!ok) { - break; - } - } - return absl::OkStatus(); - } - - absl::StatusOr> NewIterator() const override { - return std::make_unique>( - absl::MakeConstSpan(elements_)); - } - - absl::Status Append(Value value) const override { - CEL_RETURN_IF_ERROR(CheckListElement(value)); - elements_.emplace_back(std::move(value)); - return absl::OkStatus(); - } - - void Reserve(size_t capacity) const override { elements_.reserve(capacity); } - - protected: - absl::Status GetImpl( - size_t index, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { - *result = *elements_[index]; - return absl::OkStatus(); - } +struct ArenaTraits { + using constructible = std::true_type; - private: - mutable NonTrivialValueVector elements_; + using always_trivially_destructible = std::true_type; }; -class TrivialListValueBuilderImpl final : public ListValueBuilder { - public: - explicit TrivialListValueBuilderImpl(absl::Nonnull arena) - : arena_(arena), elements_(arena_) {} - - absl::Status Add(Value value) override { - CEL_RETURN_IF_ERROR(CheckListElement(value)); - elements_.emplace_back( - MakeTrivialValue(value, elements_.get_allocator().arena())); - return absl::OkStatus(); - } - - size_t Size() const override { return elements_.size(); } - - void Reserve(size_t capacity) override { elements_.reserve(capacity); } - - ListValue Build() && override { - if (elements_.empty()) { - return ListValue(); - } - return CustomListValue( - MemoryManager::Pooling(arena_).MakeShared( - std::move(elements_))); - } - - private: - absl::Nonnull const arena_; - TrivialValueVector elements_; -}; - -class NonTrivialListValueBuilderImpl final : public ListValueBuilder { - public: - NonTrivialListValueBuilderImpl() = default; - - absl::Status Add(Value value) override { - CEL_RETURN_IF_ERROR(CheckListElement(value)); - elements_.emplace_back(std::move(value)); - return absl::OkStatus(); - } - - size_t Size() const override { return elements_.size(); } - - void Reserve(size_t capacity) override { elements_.reserve(capacity); } - - ListValue Build() && override { - if (elements_.empty()) { - return ListValue(); - } - return CustomListValue( - MemoryManager::ReferenceCounting().MakeShared( - std::move(elements_))); - } - - private: - NonTrivialValueVector elements_; -}; +namespace common_internal { -} // namespace +namespace {} // namespace absl::StatusOr> MakeCompatListValue( const CustomListValue& value, absl::Nonnull descriptor_pool, absl::Nonnull message_factory, absl::Nonnull arena) { - if (value.IsEmpty()) { - return EmptyCompatListValue(); - } - TrivialValueVector vector(ArenaAllocator{arena}); - vector.reserve(value.Size()); + ListValueBuilderImpl builder(arena); + builder.Reserve(value.Size()); + CEL_RETURN_IF_ERROR(value.ForEach( [&](const Value& element) -> absl::StatusOr { - CEL_RETURN_IF_ERROR(CheckListElement(element)); - vector.push_back(MakeTrivialValue(element, arena)); + CEL_RETURN_IF_ERROR(builder.Add(element)); return true; }, descriptor_pool, message_factory, arena)); - return google::protobuf::Arena::Create(arena, std::move(vector)); + + return std::move(builder).BuildCompat(); } -Shared NewMutableListValue(Allocator<> allocator) { - if (absl::Nullable arena = allocator.arena(); - arena != nullptr) { - return MemoryManager::Pooling(arena) - .MakeShared(arena); - } - return MemoryManager::ReferenceCounting() - .MakeShared(); +Owned NewMutableListValue( + absl::Nonnull arena) { + return Owned(Owner::Arena(arena), ::new (arena->AllocateAligned( + sizeof(MutableCompatListValueImpl), + alignof(MutableCompatListValueImpl))) + MutableCompatListValueImpl(arena)); } bool IsMutableListValue(const Value& value) { @@ -738,12 +590,8 @@ const MutableListValue& GetMutableListValue(const ListValue& value) { } absl::Nonnull NewListValueBuilder( - Allocator<> allocator) { - if (absl::Nullable arena = allocator.arena(); - arena != nullptr) { - return std::make_unique(arena); - } - return std::make_unique(); + absl::Nonnull arena) { + return std::make_unique(arena); } } // namespace common_internal @@ -761,7 +609,7 @@ using ::google::api::expr::runtime::CelValue; absl::Status CheckMapValue(const Value& value) { if (auto error_value = value.AsError(); ABSL_PREDICT_FALSE(error_value)) { - return error_value->NativeValue(); + return error_value->ToStatus(); } if (auto unknown_value = value.AsUnknown(); ABSL_PREDICT_FALSE(unknown_value)) { @@ -775,9 +623,11 @@ size_t ValueHash(const Value& value) { case ValueKind::kBool: return absl::HashOf(value.kind(), value.GetBool()); case ValueKind::kInt: - return absl::HashOf(ValueKind::kInt, value.GetInt().NativeValue()); + return absl::HashOf(ValueKind::kInt, + absl::implicit_cast(value.GetInt())); case ValueKind::kUint: - return absl::HashOf(ValueKind::kUint, value.GetUint().NativeValue()); + return absl::HashOf(ValueKind::kUint, + absl::implicit_cast(value.GetUint())); case ValueKind::kString: return absl::HashOf(value.kind(), value.GetString()); default: @@ -924,7 +774,7 @@ absl::StatusOr ValueToJsonString(const Value& value) { return value.GetString().NativeString(); default: return TypeConversionError(value.GetRuntimeType(), StringType()) - .NativeValue(); + .ToStatus(); } } @@ -950,8 +800,8 @@ absl::Status MapValueToJsonObject( } for (const auto& entry : map) { - CEL_ASSIGN_OR_RETURN(auto key, ValueToJsonString(*entry.first)); - CEL_RETURN_IF_ERROR(entry.second->ConvertToJson( + CEL_ASSIGN_OR_RETURN(auto key, ValueToJsonString(entry.first)); + CEL_RETURN_IF_ERROR(entry.second.ConvertToJson( descriptor_pool, message_factory, reflection.InsertField(json, key))); } return absl::OkStatus(); @@ -975,39 +825,23 @@ absl::Status MapValueToJson( reflection.MutableStructValue(json)); } -template struct ValueHasher { using is_transparent = void; - size_t operator()(const T& value) const { return (*this)(*value); } - size_t operator()(const Value& value) const { return (ValueHash)(value); } size_t operator()(const CelValue& value) const { return (ValueHash)(value); } }; -template struct ValueEqualer { using is_transparent = void; - bool operator()(const T& lhs, const T& rhs) const { - return (*this)(*lhs, *rhs); - } - - bool operator()(const T& lhs, const Value& rhs) const { - return (*this)(*lhs, rhs); - } - - bool operator()(const Value& lhs, const T& rhs) const { - return (*this)(lhs, *rhs); - } - - bool operator()(const T& lhs, const CelValue& rhs) const { + bool operator()(const Value& lhs, const CelValue& rhs) const { return (*this)(rhs, lhs); } - bool operator()(const CelValue& lhs, const T& rhs) const { - return (CelValueEquals)(lhs, *rhs); + bool operator()(const CelValue& lhs, const Value& rhs) const { + return (CelValueEquals)(lhs, rhs); } bool operator()(const Value& lhs, const Value& rhs) const { @@ -1015,41 +849,16 @@ struct ValueEqualer { } }; -template -struct SelectValueFlatHashMapAllocator; - -template <> -struct SelectValueFlatHashMapAllocator { - using type = ArenaAllocator>; -}; - -template <> -struct SelectValueFlatHashMapAllocator { - using type = - NewDeleteAllocator>; -}; - -template -using ValueFlatHashMapAllocator = - typename SelectValueFlatHashMapAllocator::type; +using ValueFlatHashMapAllocator = ArenaAllocator>; -template using ValueFlatHashMap = - absl::flat_hash_map, ValueEqualer, - ValueFlatHashMapAllocator>; - -using TrivialValueFlatHashMapAllocator = - ValueFlatHashMapAllocator; -using NonTrivialValueFlatHashMapAllocator = - ValueFlatHashMapAllocator; + absl::flat_hash_map; -using TrivialValueFlatHashMap = ValueFlatHashMap; -using NonTrivialValueFlatHashMap = ValueFlatHashMap; - -template -class MapValueImplIterator final : public ValueIterator { +class CompatMapValueImplIterator final : public ValueIterator { public: - explicit MapValueImplIterator(absl::Nonnull*> map) + explicit CompatMapValueImplIterator( + absl::Nonnull map) : begin_(map->begin()), end_(map->end()) {} bool HasNext() override { return begin_ != end_; } @@ -1064,20 +873,68 @@ class MapValueImplIterator final : public ValueIterator { "ValueManager::Next called after ValueManager::HasNext returned " "false"); } - *result = *begin_->first; + *result = begin_->first; ++begin_; return absl::OkStatus(); } private: - typename ValueFlatHashMap::const_iterator begin_; - const typename ValueFlatHashMap::const_iterator end_; + typename ValueFlatHashMap::const_iterator begin_; + const typename ValueFlatHashMap::const_iterator end_; +}; + +class MapValueBuilderImpl final : public MapValueBuilder { + public: + explicit MapValueBuilderImpl(absl::Nonnull arena) + : arena_(arena) { + map_.Construct(arena_); + } + + ~MapValueBuilderImpl() override { + if (!entries_trivially_destructible_) { + map_.Destruct(); + } + } + + absl::Status Put(Value key, Value value) override { + CEL_RETURN_IF_ERROR(CheckMapKey(key)); + CEL_RETURN_IF_ERROR(CheckMapValue(value)); + if (auto it = map_->find(key); ABSL_PREDICT_FALSE(it != map_->end())) { + return DuplicateKeyError().ToStatus(); + } + UnsafePut(std::move(key), std::move(value)); + return absl::OkStatus(); + } + + void UnsafePut(Value key, Value value) override { + auto insertion = map_->insert({std::move(key), std::move(value)}); + ABSL_DCHECK(insertion.second); + if (entries_trivially_destructible_) { + entries_trivially_destructible_ = + ArenaTraits<>::trivially_destructible(insertion.first->first) && + ArenaTraits<>::trivially_destructible(insertion.first->second); + } + } + + size_t Size() const override { return map_->size(); } + + void Reserve(size_t capacity) override { map_->reserve(capacity); } + + MapValue Build() && override; + + CustomMapValue BuildCustom() &&; + + absl::Nonnull BuildCompat() &&; + + private: + absl::Nonnull const arena_; + internal::Manual map_; + bool entries_trivially_destructible_ = true; }; -class TrivialMapValueImpl final : public CompatMapValue { +class CompatMapValueImpl final : public CompatMapValue { public: - explicit TrivialMapValueImpl(TrivialValueFlatHashMap&& map) - : map_(std::move(map)) {} + explicit CompatMapValueImpl(ValueFlatHashMap&& map) : map_(std::move(map)) {} std::string DebugString() const override { return absl::StrCat("{", absl::StrJoin(map_, ", ", ValueFormatter{}), "}"); @@ -1098,13 +955,14 @@ class TrivialMapValueImpl final : public CompatMapValue { } CustomMapValue Clone(absl::Nonnull arena) const override { - // This is unreachable with the current logic in CustomMapValue, but could - // be called once we keep track of the owning arena in CustomListValue. - TrivialValueFlatHashMap cloned_entries(map_, - ArenaAllocator{arena}); - return CustomMapValue( - MemoryManager::Pooling(arena).MakeShared( - std::move(cloned_entries))); + ABSL_DCHECK(arena != nullptr); + + MapValueBuilderImpl builder(arena); + builder.Reserve(map_.size()); + for (const auto& entry : map_) { + builder.UnsafePut(entry.first.Clone(arena), entry.second.Clone(arena)); + } + return std::move(builder).BuildCustom(); } size_t Size() const override { return map_.size(); } @@ -1114,7 +972,8 @@ class TrivialMapValueImpl final : public CompatMapValue { absl::Nonnull message_factory, absl::Nonnull arena, absl::Nonnull result) const override { - *result = CustomListValue(MakeShared(kAdoptRef, ProjectKeys(), nullptr)); + *result = CustomListValue( + Owned(Owner::Arena(map_.get_allocator().arena()), ProjectKeys())); return absl::OkStatus(); } @@ -1124,7 +983,7 @@ class TrivialMapValueImpl final : public CompatMapValue { absl::Nonnull message_factory, absl::Nonnull arena) const override { for (const auto& entry : map_) { - CEL_ASSIGN_OR_RETURN(auto ok, callback(*entry.first, *entry.second)); + CEL_ASSIGN_OR_RETURN(auto ok, callback(entry.first, entry.second)); if (!ok) { break; } @@ -1133,7 +992,7 @@ class TrivialMapValueImpl final : public CompatMapValue { } absl::StatusOr> NewIterator() const override { - return std::make_unique>(&map_); + return std::make_unique(&map_); } absl::optional operator[](CelValue key) const override { @@ -1148,8 +1007,9 @@ class TrivialMapValueImpl final : public CompatMapValue { return absl::nullopt; } if (auto it = map_.find(key); it != map_.end()) { - return LegacyTrivialValue( - arena != nullptr ? arena : map_.get_allocator().arena(), it->second); + return common_internal::UnsafeLegacyValue( + it->second, /*stable=*/true, + arena != nullptr ? arena : map_.get_allocator().arena()); } return absl::nullopt; } @@ -1180,7 +1040,7 @@ class TrivialMapValueImpl final : public CompatMapValue { absl::Nonnull result) const override { CEL_RETURN_IF_ERROR(CheckMapKey(key)); if (auto it = map_.find(key); it != map_.end()) { - *result = *it->second; + *result = it->second; return true; } return false; @@ -1198,150 +1058,57 @@ class TrivialMapValueImpl final : public CompatMapValue { private: absl::Nonnull ProjectKeys() const { absl::call_once(keys_once_, [this]() { - TrivialValueVector elements(map_.get_allocator().arena()); - elements.reserve(map_.size()); + ListValueBuilderImpl builder(map_.get_allocator().arena()); + builder.Reserve(map_.size()); + for (const auto& entry : map_) { - elements.push_back(entry.first); + builder.UnsafeAdd(entry.first); } - ::new (static_cast(&keys_[0])) - TrivialListValueImpl(std::move(elements)); + + std::move(builder).BuildCompatAt(&keys_[0]); }); return std::launder( - reinterpret_cast(&keys_[0])); + reinterpret_cast(&keys_[0])); } - const TrivialValueFlatHashMap map_; + const ValueFlatHashMap map_; mutable absl::once_flag keys_once_; - alignas( - TrivialListValueImpl) mutable char keys_[sizeof(TrivialListValueImpl)]; -}; - -} // namespace - -} // namespace common_internal - -template <> -struct NativeTypeTraits { - static bool SkipDestructor(const common_internal::TrivialMapValueImpl&) { - return true; - } + alignas(CompatListValueImpl) mutable char keys_[sizeof(CompatListValueImpl)]; }; -namespace common_internal { - -namespace { - -class NonTrivialMapValueImpl final : public CustomMapValueInterface { - public: - explicit NonTrivialMapValueImpl(NonTrivialValueFlatHashMap&& map) - : map_(std::move(map)) {} - - std::string DebugString() const override { - return absl::StrCat("{", absl::StrJoin(map_, ", ", ValueFormatter{}), "}"); - } - - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return MapValueToJson(map_, descriptor_pool, message_factory, json); - } - - absl::Status ConvertToJsonObject( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return MapValueToJsonObject(map_, descriptor_pool, message_factory, json); - } - - CustomMapValue Clone(absl::Nonnull arena) const override { - // This is unreachable with the current logic in CustomMapValue, but could - // be called once we keep track of the owning arena in CustomListValue. - TrivialValueFlatHashMap cloned_entries(ArenaAllocator{arena}); - cloned_entries.reserve(map_.size()); - for (const auto& entry : map_) { - const auto inserted = - cloned_entries - .insert_or_assign(MakeTrivialValue(*entry.first, arena), - MakeTrivialValue(*entry.second, arena)) - .second; - ABSL_DCHECK(inserted); - } - return CustomMapValue( - MemoryManager::Pooling(arena).MakeShared( - std::move(cloned_entries))); - } - - size_t Size() const override { return map_.size(); } - - absl::Status ListKeys( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { - auto builder = NewListValueBuilder(arena); - builder->Reserve(Size()); - for (const auto& entry : map_) { - CEL_RETURN_IF_ERROR(builder->Add(*entry.first)); - } - *result = std::move(*builder).Build(); - return absl::OkStatus(); - } - - absl::Status ForEach( - ForEachCallback callback, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena) const override { - for (const auto& entry : map_) { - CEL_ASSIGN_OR_RETURN(auto ok, callback(*entry.first, *entry.second)); - if (!ok) { - break; - } - } - return absl::OkStatus(); - } - - absl::StatusOr> NewIterator() const override { - return std::make_unique>(&map_); +MapValue MapValueBuilderImpl::Build() && { + if (map_->empty()) { + return MapValue(); } + return std::move(*this).BuildCustom(); +} - protected: - absl::StatusOr FindImpl( - const Value& key, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { - CEL_RETURN_IF_ERROR(CheckMapKey(key)); - if (auto it = map_.find(key); it != map_.end()) { - *result = *it->second; - return true; - } - return false; +CustomMapValue MapValueBuilderImpl::BuildCustom() && { + if (map_->empty()) { + return CustomMapValue(Owned(Owner::Arena(arena_), EmptyCompatMapValue())); } + return CustomMapValue( + Owned(Owner::Arena(arena_), std::move(*this).BuildCompat())); +} - absl::StatusOr HasImpl( - const Value& key, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena) const override { - CEL_RETURN_IF_ERROR(CheckMapKey(key)); - return map_.find(key) != map_.end(); +absl::Nonnull MapValueBuilderImpl::BuildCompat() && { + if (map_->empty()) { + return EmptyCompatMapValue(); } - - private: - NativeTypeId GetNativeTypeId() const override { - return NativeTypeId::For(); + absl::Nonnull impl = ::new (arena_->AllocateAligned( + sizeof(CompatMapValueImpl), alignof(CompatMapValueImpl))) + CompatMapValueImpl(std::move(*map_)); + if (!entries_trivially_destructible_) { + arena_->OwnDestructor(impl); + entries_trivially_destructible_ = true; } - - const NonTrivialValueFlatHashMap map_; -}; + return impl; +} class TrivialMutableMapValueImpl final : public MutableCompatMapValue { public: explicit TrivialMutableMapValueImpl(absl::Nonnull arena) - : map_(TrivialValueFlatHashMapAllocator{arena}) {} + : map_(arena) {} std::string DebugString() const override { return absl::StrCat("{", absl::StrJoin(map_, ", ", ValueFormatter{}), "}"); @@ -1362,13 +1129,14 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { } CustomMapValue Clone(absl::Nonnull arena) const override { - // This is unreachable with the current logic in CustomMapValue, but could - // be called once we keep track of the owning arena in CustomListValue. - TrivialValueFlatHashMap cloned_entries(map_, - ArenaAllocator{arena}); - return CustomMapValue( - MemoryManager::Pooling(arena).MakeShared( - std::move(cloned_entries))); + ABSL_DCHECK(arena != nullptr); + + MapValueBuilderImpl builder(arena); + builder.Reserve(map_.size()); + for (const auto& entry : map_) { + builder.UnsafePut(entry.first.Clone(arena), entry.second.Clone(arena)); + } + return std::move(builder).BuildCustom(); } size_t Size() const override { return map_.size(); } @@ -1378,7 +1146,8 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { absl::Nonnull message_factory, absl::Nonnull arena, absl::Nonnull result) const override { - *result = CustomListValue(MakeShared(kAdoptRef, ProjectKeys(), nullptr)); + *result = CustomListValue( + Owned(Owner::Arena(map_.get_allocator().arena()), ProjectKeys())); return absl::OkStatus(); } @@ -1388,7 +1157,7 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { absl::Nonnull message_factory, absl::Nonnull arena) const override { for (const auto& entry : map_) { - CEL_ASSIGN_OR_RETURN(auto ok, callback(*entry.first, *entry.second)); + CEL_ASSIGN_OR_RETURN(auto ok, callback(entry.first, entry.second)); if (!ok) { break; } @@ -1397,7 +1166,7 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { } absl::StatusOr> NewIterator() const override { - return std::make_unique>(&map_); + return std::make_unique(&map_); } absl::optional operator[](CelValue key) const override { @@ -1412,8 +1181,9 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { return absl::nullopt; } if (auto it = map_.find(key); it != map_.end()) { - return LegacyTrivialValue( - arena != nullptr ? arena : map_.get_allocator().arena(), it->second); + return common_internal::UnsafeLegacyValue( + it->second, /*stable=*/false, + arena != nullptr ? arena : map_.get_allocator().arena()); } return absl::nullopt; } @@ -1439,13 +1209,19 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { CEL_RETURN_IF_ERROR(CheckMapKey(key)); CEL_RETURN_IF_ERROR(CheckMapValue(value)); if (auto it = map_.find(key); ABSL_PREDICT_FALSE(it != map_.end())) { - return DuplicateKeyError().NativeValue(); + return DuplicateKeyError().ToStatus(); + } + auto insertion = map_.insert({std::move(key), std::move(value)}); + ABSL_DCHECK(insertion.second); + if (entries_trivially_destructible_) { + entries_trivially_destructible_ = + ArenaTraits<>::trivially_destructible(insertion.first->first) && + ArenaTraits<>::trivially_destructible(insertion.first->second); + if (!entries_trivially_destructible_) { + map_.get_allocator().arena()->OwnDestructor( + const_cast(this)); + } } - absl::Nonnull arena = map_.get_allocator().arena(); - auto inserted = map_.insert(std::pair{MakeTrivialValue(key, arena), - MakeTrivialValue(value, arena)}) - .second; - ABSL_DCHECK(inserted); return absl::OkStatus(); } @@ -1460,7 +1236,7 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { absl::Nonnull result) const override { CEL_RETURN_IF_ERROR(CheckMapKey(key)); if (auto it = map_.find(key); it != map_.end()) { - *result = *it->second; + *result = it->second; return true; } return false; @@ -1478,226 +1254,23 @@ class TrivialMutableMapValueImpl final : public MutableCompatMapValue { private: absl::Nonnull ProjectKeys() const { absl::call_once(keys_once_, [this]() { - TrivialValueVector elements(map_.get_allocator().arena()); - elements.reserve(map_.size()); + ListValueBuilderImpl builder(map_.get_allocator().arena()); + builder.Reserve(map_.size()); + for (const auto& entry : map_) { - elements.push_back(entry.first); + builder.UnsafeAdd(entry.first); } - ::new (static_cast(&keys_[0])) - TrivialListValueImpl(std::move(elements)); + + std::move(builder).BuildCompatAt(&keys_[0]); }); return std::launder( - reinterpret_cast(&keys_[0])); + reinterpret_cast(&keys_[0])); } - mutable TrivialValueFlatHashMap map_; + mutable ValueFlatHashMap map_; + mutable bool entries_trivially_destructible_ = true; mutable absl::once_flag keys_once_; - alignas( - TrivialListValueImpl) mutable char keys_[sizeof(TrivialListValueImpl)]; -}; - -} // namespace - -} // namespace common_internal - -template <> -struct NativeTypeTraits { - static bool SkipDestructor( - const common_internal::TrivialMutableMapValueImpl&) { - return true; - } -}; - -namespace common_internal { - -namespace { - -class NonTrivialMutableMapValueImpl final : public MutableMapValue { - public: - NonTrivialMutableMapValueImpl() = default; - - std::string DebugString() const override { - return absl::StrCat("{", absl::StrJoin(map_, ", ", ValueFormatter{}), "}"); - } - - absl::Status ConvertToJson( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return MapValueToJson(map_, descriptor_pool, message_factory, json); - } - - absl::Status ConvertToJsonObject( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull json) const override { - return MapValueToJsonObject(map_, descriptor_pool, message_factory, json); - } - - CustomMapValue Clone(absl::Nonnull arena) const override { - // This is unreachable with the current logic in CustomMapValue, but could - // be called once we keep track of the owning arena in CustomListValue. - TrivialValueFlatHashMap cloned_entries(ArenaAllocator{arena}); - cloned_entries.reserve(map_.size()); - for (const auto& entry : map_) { - const auto inserted = - cloned_entries - .insert_or_assign(MakeTrivialValue(*entry.first, arena), - MakeTrivialValue(*entry.second, arena)) - .second; - ABSL_DCHECK(inserted); - } - return CustomMapValue( - MemoryManager::Pooling(arena).MakeShared( - std::move(cloned_entries))); - } - - size_t Size() const override { return map_.size(); } - - absl::Status ListKeys( - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { - auto builder = NewListValueBuilder(arena); - builder->Reserve(Size()); - for (const auto& entry : map_) { - CEL_RETURN_IF_ERROR(builder->Add(*entry.first)); - } - *result = std::move(*builder).Build(); - return absl::OkStatus(); - } - - absl::Status ForEach( - ForEachCallback callback, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena) const override { - for (const auto& entry : map_) { - CEL_ASSIGN_OR_RETURN(auto ok, callback(*entry.first, *entry.second)); - if (!ok) { - break; - } - } - return absl::OkStatus(); - } - - absl::StatusOr> NewIterator() const override { - return std::make_unique>(&map_); - } - - absl::Status Put(Value key, Value value) const override { - CEL_RETURN_IF_ERROR(CheckMapKey(key)); - CEL_RETURN_IF_ERROR(CheckMapValue(value)); - if (auto inserted = - map_.insert(std::pair{NonTrivialValue(std::move(key)), - NonTrivialValue(std::move(value))}) - .second; - !inserted) { - return DuplicateKeyError().NativeValue(); - } - return absl::OkStatus(); - } - - void Reserve(size_t capacity) const override { map_.reserve(capacity); } - - protected: - absl::StatusOr FindImpl( - const Value& key, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena, - absl::Nonnull result) const override { - CEL_RETURN_IF_ERROR(CheckMapKey(key)); - if (auto it = map_.find(key); it != map_.end()) { - *result = *it->second; - return true; - } - return false; - } - - absl::StatusOr HasImpl( - const Value& key, - absl::Nonnull descriptor_pool, - absl::Nonnull message_factory, - absl::Nonnull arena) const override { - CEL_RETURN_IF_ERROR(CheckMapKey(key)); - return map_.find(key) != map_.end(); - } - - private: - mutable NonTrivialValueFlatHashMap map_; -}; - -class TrivialMapValueBuilderImpl final : public MapValueBuilder { - public: - explicit TrivialMapValueBuilderImpl(absl::Nonnull arena) - : arena_(arena), map_(arena_) {} - - absl::Status Put(Value key, Value value) override { - CEL_RETURN_IF_ERROR(CheckMapKey(key)); - CEL_RETURN_IF_ERROR(CheckMapValue(value)); - if (auto it = map_.find(key); ABSL_PREDICT_FALSE(it != map_.end())) { - return DuplicateKeyError().NativeValue(); - } - absl::Nonnull arena = map_.get_allocator().arena(); - auto inserted = map_.insert(std::pair{MakeTrivialValue(key, arena), - MakeTrivialValue(value, arena)}) - .second; - ABSL_DCHECK(inserted); - return absl::OkStatus(); - } - - size_t Size() const override { return map_.size(); } - - void Reserve(size_t capacity) override { map_.reserve(capacity); } - - MapValue Build() && override { - if (map_.empty()) { - return MapValue(); - } - return CustomMapValue( - MemoryManager::Pooling(arena_).MakeShared( - std::move(map_))); - } - - private: - absl::Nonnull const arena_; - TrivialValueFlatHashMap map_; -}; - -class NonTrivialMapValueBuilderImpl final : public MapValueBuilder { - public: - NonTrivialMapValueBuilderImpl() = default; - - absl::Status Put(Value key, Value value) override { - CEL_RETURN_IF_ERROR(CheckMapKey(key)); - CEL_RETURN_IF_ERROR(CheckMapValue(value)); - if (auto inserted = - map_.insert(std::pair{NonTrivialValue(std::move(key)), - NonTrivialValue(std::move(value))}) - .second; - !inserted) { - return DuplicateKeyError().NativeValue(); - } - return absl::OkStatus(); - } - - size_t Size() const override { return map_.size(); } - - void Reserve(size_t capacity) override { map_.reserve(capacity); } - - MapValue Build() && override { - if (map_.empty()) { - return MapValue(); - } - return CustomMapValue( - MemoryManager::ReferenceCounting().MakeShared( - std::move(map_))); - } - - private: - NonTrivialValueFlatHashMap map_; + alignas(CompatListValueImpl) mutable char keys_[sizeof(CompatListValueImpl)]; }; } // namespace @@ -1707,34 +1280,24 @@ absl::StatusOr> MakeCompatMapValue( absl::Nonnull descriptor_pool, absl::Nonnull message_factory, absl::Nonnull arena) { - if (value.IsEmpty()) { - return EmptyCompatMapValue(); - } - TrivialValueFlatHashMap map(TrivialValueFlatHashMapAllocator{arena}); - map.reserve(value.Size()); + MapValueBuilderImpl builder(arena); + builder.Reserve(value.Size()); + CEL_RETURN_IF_ERROR(value.ForEach( [&](const Value& key, const Value& value) -> absl::StatusOr { - CEL_RETURN_IF_ERROR(CheckMapKey(key)); - CEL_RETURN_IF_ERROR(CheckMapValue(value)); - const auto inserted = - map.insert_or_assign(MakeTrivialValue(key, arena), - MakeTrivialValue(value, arena)) - .second; - ABSL_DCHECK(inserted); + CEL_RETURN_IF_ERROR(builder.Put(key, value)); return true; }, descriptor_pool, message_factory, arena)); - return google::protobuf::Arena::Create(arena, std::move(map)); + + return std::move(builder).BuildCompat(); } -Shared NewMutableMapValue(Allocator<> allocator) { - if (absl::Nullable arena = allocator.arena(); - arena != nullptr) { - return MemoryManager::Pooling(arena).MakeShared( - arena); - } - return MemoryManager::ReferenceCounting() - .MakeShared(); +Owned NewMutableMapValue(absl::Nonnull arena) { + return Owned(Owner::Arena(arena), ::new (arena->AllocateAligned( + sizeof(TrivialMutableMapValueImpl), + alignof(TrivialMutableMapValueImpl))) + TrivialMutableMapValueImpl(arena)); } bool IsMutableMapValue(const Value& value) { @@ -1819,12 +1382,8 @@ const MutableMapValue& GetMutableMapValue(const MapValue& value) { } absl::Nonnull NewMapValueBuilder( - Allocator<> allocator) { - if (absl::Nullable arena = allocator.arena(); - arena != nullptr) { - return std::make_unique(arena); - } - return std::make_unique(); + absl::Nonnull arena) { + return std::make_unique(arena); } } // namespace common_internal diff --git a/eval/tests/modern_benchmark_test.cc b/eval/tests/modern_benchmark_test.cc index 52aecd0ca..592a8c2b2 100644 --- a/eval/tests/modern_benchmark_test.cc +++ b/eval/tests/modern_benchmark_test.cc @@ -36,7 +36,6 @@ #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" #include "absl/strings/match.h" -#include "absl/types/optional.h" #include "common/allocator.h" #include "common/casting.h" #include "common/memory.h" @@ -400,8 +399,7 @@ class RequestMapImpl : public CustomMapValueInterface { } CustomMapValue Clone(absl::Nonnull arena) const override { - return CustomMapValue( - MemoryManager::Pooling(arena).MakeShared()); + return CustomMapValue(AllocateShared(arena)); } protected: @@ -463,8 +461,7 @@ void BM_PolicySymbolicMap(benchmark::State& state) { *runtime, parsed_expr)); Activation activation; - CustomMapValue map_value( - MemoryManager::Pooling(&arena).MakeShared()); + CustomMapValue map_value(AllocateShared(&arena)); activation.InsertOrAssignValue("request", std::move(map_value)); diff --git a/internal/BUILD b/internal/BUILD index 66d4e9646..84b8d52d4 100644 --- a/internal/BUILD +++ b/internal/BUILD @@ -736,3 +736,12 @@ cc_library( hdrs = ["noop_delete.h"], deps = ["@com_google_absl//absl/base:nullability"], ) + +cc_library( + name = "manual", + hdrs = ["manual.h"], + deps = [ + "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/base:nullability", + ], +) diff --git a/internal/manual.h b/internal/manual.h new file mode 100644 index 000000000..19c20bf08 --- /dev/null +++ b/internal/manual.h @@ -0,0 +1,91 @@ +// Copyright 2025 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef THIRD_PARTY_CEL_CPP_INTERNAL_MANUAL_H_ +#define THIRD_PARTY_CEL_CPP_INTERNAL_MANUAL_H_ + +#include +#include + +#include "absl/base/attributes.h" +#include "absl/base/nullability.h" + +namespace cel::internal { + +template +class Manual final { + public: + static_assert(!std::is_reference_v, "T must not be a reference"); + static_assert(!std::is_array_v, "T must not be an array"); + static_assert(!std::is_const_v, "T must not be const qualified"); + static_assert(!std::is_volatile_v, "T must not be volatile qualified"); + + using element_type = T; + + Manual() = default; + + Manual(const Manual&) = delete; + Manual(Manual&&) = delete; + + ~Manual() = default; + + Manual& operator=(const Manual&) = delete; + Manual& operator=(Manual&&) = delete; + + constexpr absl::Nonnull get() ABSL_ATTRIBUTE_LIFETIME_BOUND { + return std::launder(reinterpret_cast(&storage_[0])); + } + + constexpr absl::Nonnull get() const ABSL_ATTRIBUTE_LIFETIME_BOUND { + return std::launder(reinterpret_cast(&storage_[0])); + } + + constexpr T& operator*() ABSL_ATTRIBUTE_LIFETIME_BOUND { return *get(); } + + constexpr const T& operator*() const ABSL_ATTRIBUTE_LIFETIME_BOUND { + return *get(); + } + + constexpr absl::Nonnull operator->() ABSL_ATTRIBUTE_LIFETIME_BOUND { + return get(); + } + + constexpr absl::Nonnull operator->() const + ABSL_ATTRIBUTE_LIFETIME_BOUND { + return get(); + } + + template + absl::Nonnull Construct(Args&&... args) ABSL_ATTRIBUTE_LIFETIME_BOUND { + return ::new (static_cast(&storage_[0])) + T(std::forward(args)...); + } + + absl::Nonnull DefaultConstruct() { + return ::new (static_cast(&storage_[0])) T; + } + + absl::Nonnull ValueConstruct() { + return ::new (static_cast(&storage_[0])) T(); + } + + void Destruct() { get()->~T(); } + + private: + alignas(T) char storage_[sizeof(T)]; +}; + +} // namespace cel::internal + +#endif // THIRD_PARTY_CEL_CPP_INTERNAL_MANUAL_H_