diff --git a/common/BUILD b/common/BUILD index f75c82f17..a41f0cb1a 100644 --- a/common/BUILD +++ b/common/BUILD @@ -506,7 +506,6 @@ cc_library( ":allocator", "//internal:to_address", "@com_google_absl//absl/base:core_headers", - "@com_google_absl//absl/base:no_destructor", "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/numeric:bits", diff --git a/common/memory.cc b/common/memory.cc index c00c12ed8..46373ba5b 100644 --- a/common/memory.cc +++ b/common/memory.cc @@ -19,10 +19,8 @@ #include #include -#include "absl/base/no_destructor.h" #include "absl/log/absl_check.h" #include "absl/numeric/bits.h" -#include "google/protobuf/arena.h" namespace cel { @@ -73,11 +71,4 @@ bool ReferenceCountingMemoryManager::Deallocate(void* ptr, size_t size, return true; } -MemoryManager MemoryManager::Unmanaged() { - // A static singleton arena, using `absl::NoDestructor` to avoid warnings - // related static variables without trivial destructors. - static absl::NoDestructor arena; - return MemoryManager::Pooling(&*arena); -} - } // namespace cel diff --git a/common/memory.h b/common/memory.h index 967bc57e2..a9ae46023 100644 --- a/common/memory.h +++ b/common/memory.h @@ -152,17 +152,6 @@ class PoolingMemoryManager final { // resources. class MemoryManager final { public: - // Returns a `MemoryManager` which utilizes an arena but never frees its - // memory. It is effectively a memory leak and should only be used for limited - // use cases, such as initializing singletons which live for the life of the - // program. - static MemoryManager Unmanaged(); - - // Returns a `MemoryManager` which utilizes reference counting. - ABSL_MUST_USE_RESULT static MemoryManager ReferenceCounting() { - return MemoryManager(nullptr); - } - // Returns a `MemoryManager` which utilizes an arena. ABSL_MUST_USE_RESULT static MemoryManager Pooling( google::protobuf::Arena* absl_nonnull arena) { diff --git a/eval/public/BUILD b/eval/public/BUILD index 2172774a9..f432767fa 100644 --- a/eval/public/BUILD +++ b/eval/public/BUILD @@ -96,7 +96,6 @@ cc_library( "//common:native_type", "//eval/internal:errors", "//eval/public/structs:legacy_type_info_apis", - "//extensions/protobuf:memory_manager", "//internal:casts", "//internal:status_macros", "//internal:utf8", diff --git a/eval/public/cel_value.cc b/eval/public/cel_value.cc index 7e2d3b5d4..33b78c1c9 100644 --- a/eval/public/cel_value.cc +++ b/eval/public/cel_value.cc @@ -14,10 +14,8 @@ #include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" -#include "common/memory.h" #include "eval/internal/errors.h" #include "eval/public/structs/legacy_type_info_apis.h" -#include "extensions/protobuf/memory_manager.h" #include "google/protobuf/arena.h" namespace google::api::expr::runtime { @@ -286,23 +284,6 @@ CelValue CelValue::CreateList() { return CreateList(EmptyCelList::Get()); } CelValue CelValue::CreateMap() { return CreateMap(EmptyCelMap::Get()); } -CelValue CreateErrorValue(cel::MemoryManagerRef manager, - absl::string_view message, - absl::StatusCode error_code) { - // TODO(uncreated-issue/1): assume arena-style allocator while migrating to new - // value type. - Arena* arena = cel::extensions::ProtoMemoryManagerArena(manager); - return CreateErrorValue(arena, message, error_code); -} - -CelValue CreateErrorValue(cel::MemoryManagerRef manager, - const absl::Status& status) { - // TODO(uncreated-issue/1): assume arena-style allocator while migrating to new - // value type. - Arena* arena = cel::extensions::ProtoMemoryManagerArena(manager); - return CreateErrorValue(arena, status); -} - CelValue CreateErrorValue(Arena* arena, absl::string_view message, absl::StatusCode error_code) { CelError* error = Arena::Create(arena, error_code, message); @@ -314,12 +295,6 @@ CelValue CreateErrorValue(Arena* arena, const absl::Status& status) { return CelValue::CreateError(error); } -CelValue CreateNoMatchingOverloadError(cel::MemoryManagerRef manager, - absl::string_view fn) { - return CelValue::CreateError(interop::CreateNoMatchingOverloadError( - cel::extensions::ProtoMemoryManagerArena(manager), fn)); -} - CelValue CreateNoMatchingOverloadError(google::protobuf::Arena* arena, absl::string_view fn) { return CelValue::CreateError( @@ -333,22 +308,10 @@ bool CheckNoMatchingOverloadError(CelValue value) { cel::runtime_internal::kErrNoMatchingOverload); } -CelValue CreateNoSuchFieldError(cel::MemoryManagerRef manager, - absl::string_view field) { - return CelValue::CreateError(interop::CreateNoSuchFieldError( - cel::extensions::ProtoMemoryManagerArena(manager), field)); -} - CelValue CreateNoSuchFieldError(google::protobuf::Arena* arena, absl::string_view field) { return CelValue::CreateError(interop::CreateNoSuchFieldError(arena, field)); } -CelValue CreateNoSuchKeyError(cel::MemoryManagerRef manager, - absl::string_view key) { - return CelValue::CreateError(interop::CreateNoSuchKeyError( - cel::extensions::ProtoMemoryManagerArena(manager), key)); -} - CelValue CreateNoSuchKeyError(google::protobuf::Arena* arena, absl::string_view key) { return CelValue::CreateError(interop::CreateNoSuchKeyError(arena, key)); } @@ -365,15 +328,6 @@ CelValue CreateMissingAttributeError(google::protobuf::Arena* arena, interop::CreateMissingAttributeError(arena, missing_attribute_path)); } -CelValue CreateMissingAttributeError(cel::MemoryManagerRef manager, - absl::string_view missing_attribute_path) { - // TODO(uncreated-issue/1): assume arena-style allocator while migrating - // to new value type. - return CelValue::CreateError(interop::CreateMissingAttributeError( - cel::extensions::ProtoMemoryManagerArena(manager), - missing_attribute_path)); -} - bool IsMissingAttributeError(const CelValue& value) { const CelError* error; if (!value.GetValue(&error)) return false; @@ -385,12 +339,6 @@ bool IsMissingAttributeError(const CelValue& value) { return false; } -CelValue CreateUnknownFunctionResultError(cel::MemoryManagerRef manager, - absl::string_view help_message) { - return CelValue::CreateError(interop::CreateUnknownFunctionResultError( - cel::extensions::ProtoMemoryManagerArena(manager), help_message)); -} - CelValue CreateUnknownFunctionResultError(google::protobuf::Arena* arena, absl::string_view help_message) { return CelValue::CreateError( diff --git a/eval/public/cel_value.h b/eval/public/cel_value.h index 5bf50d01a..2a98414f3 100644 --- a/eval/public/cel_value.h +++ b/eval/public/cel_value.h @@ -653,44 +653,54 @@ class CelMap { // Utility method that generates CelValue containing CelError. // message an error message // error_code error code -CelValue CreateErrorValue( - cel::MemoryManagerRef manager ABSL_ATTRIBUTE_LIFETIME_BOUND, - absl::string_view message, - absl::StatusCode error_code = absl::StatusCode::kUnknown); CelValue CreateErrorValue( google::protobuf::Arena* arena, absl::string_view message, absl::StatusCode error_code = absl::StatusCode::kUnknown); - -// Utility method for generating a CelValue from an absl::Status. -CelValue CreateErrorValue(cel::MemoryManagerRef manager - ABSL_ATTRIBUTE_LIFETIME_BOUND, - const absl::Status& status); +ABSL_DEPRECATE_AND_INLINE() +inline CelValue CreateErrorValue( + cel::MemoryManagerRef manager ABSL_ATTRIBUTE_LIFETIME_BOUND, + absl::string_view message, + absl::StatusCode error_code = absl::StatusCode::kUnknown) { + return CreateErrorValue(manager.arena(), message, error_code); +} // Utility method for generating a CelValue from an absl::Status. CelValue CreateErrorValue(google::protobuf::Arena* arena, const absl::Status& status); +ABSL_DEPRECATE_AND_INLINE() +inline CelValue CreateErrorValue(cel::MemoryManagerRef manager + ABSL_ATTRIBUTE_LIFETIME_BOUND, + const absl::Status& status) { + return CreateErrorValue(manager.arena(), status); +} // Create an error for failed overload resolution, optionally including the name // of the function. -CelValue CreateNoMatchingOverloadError(cel::MemoryManagerRef manager - ABSL_ATTRIBUTE_LIFETIME_BOUND, - absl::string_view fn = ""); -ABSL_DEPRECATED("Prefer using the generic MemoryManager overload") CelValue CreateNoMatchingOverloadError(google::protobuf::Arena* arena, absl::string_view fn = ""); +ABSL_DEPRECATE_AND_INLINE() +inline CelValue CreateNoMatchingOverloadError(cel::MemoryManagerRef manager + ABSL_ATTRIBUTE_LIFETIME_BOUND, + absl::string_view fn = "") { + return CreateNoMatchingOverloadError(manager.arena(), fn); +} bool CheckNoMatchingOverloadError(CelValue value); -CelValue CreateNoSuchFieldError(cel::MemoryManagerRef manager - ABSL_ATTRIBUTE_LIFETIME_BOUND, - absl::string_view field = ""); -ABSL_DEPRECATED("Prefer using the generic MemoryManager overload") CelValue CreateNoSuchFieldError(google::protobuf::Arena* arena, absl::string_view field = ""); +ABSL_DEPRECATE_AND_INLINE() +inline CelValue CreateNoSuchFieldError(cel::MemoryManagerRef manager + ABSL_ATTRIBUTE_LIFETIME_BOUND, + absl::string_view field = "") { + return CreateNoSuchFieldError(manager.arena(), field); +} -CelValue CreateNoSuchKeyError(cel::MemoryManagerRef manager - ABSL_ATTRIBUTE_LIFETIME_BOUND, - absl::string_view key); -ABSL_DEPRECATED("Prefer using the generic MemoryManager overload") CelValue CreateNoSuchKeyError(google::protobuf::Arena* arena, absl::string_view key); +ABSL_DEPRECATE_AND_INLINE() +inline CelValue CreateNoSuchKeyError(cel::MemoryManagerRef manager + ABSL_ATTRIBUTE_LIFETIME_BOUND, + absl::string_view key) { + return CreateNoSuchKeyError(manager.arena(), key); +} bool CheckNoSuchKeyError(CelValue value); @@ -698,12 +708,14 @@ bool CheckNoSuchKeyError(CelValue value); // value is undefined. For example, this may represent a field in a proto // message bound to the activation whose value can't be determined by the // hosting application. -CelValue CreateMissingAttributeError(cel::MemoryManagerRef manager - ABSL_ATTRIBUTE_LIFETIME_BOUND, - absl::string_view missing_attribute_path); -ABSL_DEPRECATED("Prefer using the generic MemoryManager overload") CelValue CreateMissingAttributeError(google::protobuf::Arena* arena, absl::string_view missing_attribute_path); +ABSL_DEPRECATE_AND_INLINE() +inline CelValue CreateMissingAttributeError( + cel::MemoryManagerRef manager ABSL_ATTRIBUTE_LIFETIME_BOUND, + absl::string_view missing_attribute_path) { + return CreateMissingAttributeError(manager.arena(), missing_attribute_path); +} ABSL_CONST_INIT extern const absl::string_view kPayloadUrlMissingAttributePath; bool IsMissingAttributeError(const CelValue& value); @@ -711,12 +723,14 @@ bool IsMissingAttributeError(const CelValue& value); // Returns error indicating the result of the function is unknown. This is used // as a signal to create an unknown set if unknown function handling is opted // into. -CelValue CreateUnknownFunctionResultError(cel::MemoryManagerRef manager - ABSL_ATTRIBUTE_LIFETIME_BOUND, - absl::string_view help_message); -ABSL_DEPRECATED("Prefer using the generic MemoryManager overload") CelValue CreateUnknownFunctionResultError(google::protobuf::Arena* arena, absl::string_view help_message); +ABSL_DEPRECATE_AND_INLINE() +inline CelValue CreateUnknownFunctionResultError( + cel::MemoryManagerRef manager ABSL_ATTRIBUTE_LIFETIME_BOUND, + absl::string_view help_message) { + return CreateUnknownFunctionResultError(manager.arena(), help_message); +} // Returns true if this is unknown value error indicating that evaluation // called an extension function whose value is unknown for the given args. diff --git a/extensions/protobuf/memory_manager.cc b/extensions/protobuf/memory_manager.cc index 5b3e6e74b..0953d0a45 100644 --- a/extensions/protobuf/memory_manager.cc +++ b/extensions/protobuf/memory_manager.cc @@ -23,8 +23,7 @@ namespace cel { namespace extensions { MemoryManagerRef ProtoMemoryManager(google::protobuf::Arena* arena) { - return arena != nullptr ? MemoryManagerRef::Pooling(arena) - : MemoryManagerRef::ReferenceCounting(); + return MemoryManagerRef::Pooling(arena); } google::protobuf::Arena* absl_nullable ProtoMemoryManagerArena( diff --git a/extensions/protobuf/memory_manager_test.cc b/extensions/protobuf/memory_manager_test.cc index ddab4cf32..956842bb9 100644 --- a/extensions/protobuf/memory_manager_test.cc +++ b/extensions/protobuf/memory_manager_test.cc @@ -22,7 +22,6 @@ namespace cel::extensions { namespace { using ::testing::Eq; -using ::testing::IsNull; using ::testing::NotNull; TEST(ProtoMemoryManager, MemoryManagement) { @@ -41,17 +40,12 @@ TEST(ProtoMemoryManagerRef, MemoryManagement) { google::protobuf::Arena arena; auto memory_manager = ProtoMemoryManagerRef(&arena); EXPECT_EQ(memory_manager.memory_management(), MemoryManagement::kPooling); - memory_manager = ProtoMemoryManagerRef(nullptr); - EXPECT_EQ(memory_manager.memory_management(), - MemoryManagement::kReferenceCounting); } TEST(ProtoMemoryManagerRef, Arena) { google::protobuf::Arena arena; auto memory_manager = ProtoMemoryManagerRef(&arena); EXPECT_THAT(ProtoMemoryManagerArena(memory_manager), Eq(&arena)); - memory_manager = ProtoMemoryManagerRef(nullptr); - EXPECT_THAT(ProtoMemoryManagerArena(memory_manager), IsNull()); } } // namespace