From 57533e29a30b71e4b84d923af2519afa16cd98e8 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Tue, 6 Oct 2026 15:53:44 -0700 Subject: [PATCH] Eliminate temporary `std::string` copies in CEL protobuf map and `Struct` lookups. Previously, looking up or checking presence of a string key in a protobuf map or `google.protobuf.Struct` allocated or constructed a temporary `std::string` via `StringValue::NativeString()` (or `std::string(str_key.value())`) before populating a `google::protobuf::MapKey`, even when the key was already backed by a contiguous `absl::string_view` and only a stack scratch buffer was needed for fragmented cord-backed strings. This change uses `StringValue::ToStringView(&scratch)` (and direct `absl::string_view` in `DynamicMap` and `LookupMapValue`) before calling `MapKey::SetStringValue`, avoiding heap allocations and redundant copies for string-keyed map and `google.protobuf.Struct` field accesses across: - `ParsedMapFieldValue::Find` and `ParsedMapFieldValue::Has` - `ProtoStringMapKeyFromValueConverter` - `FieldBackedMapImpl::LookupMapValue` - `DynamicMap::Has` and `DynamicMap::operator[]` (`cel_proto_wrap_util.cc`) - `MapKeyFromQualifier` and `ProtoQualifyState::ApplySelectQualifiers` PiperOrigin-RevId: 994679246 --- common/values/parsed_map_field_value.cc | 5 +++-- common/values/struct_value_builder.cc | 5 +++-- eval/public/containers/internal_field_backed_map_impl.cc | 4 +--- eval/public/structs/cel_proto_wrap_util.cc | 4 ++-- extensions/protobuf/internal/qualify.cc | 4 +--- extensions/select_optimization.cc | 3 ++- 6 files changed, 12 insertions(+), 13 deletions(-) diff --git a/common/values/parsed_map_field_value.cc b/common/values/parsed_map_field_value.cc index e17bfb4d2..7324286fc 100644 --- a/common/values/parsed_map_field_value.cc +++ b/common/values/parsed_map_field_value.cc @@ -304,8 +304,9 @@ bool ValueToProtoMapKey(const Value& key, } case google::protobuf::FieldDescriptor::CPPTYPE_STRING: { if (auto string_key = key.AsString(); string_key) { - proto_key_scratch = string_key->NativeString(); - proto_key->SetStringValue(proto_key_scratch); + // `google::protobuf::MapKey` does not own the string, which may be stored inline + // in `key` or in `proto_key_scratch`; `key` must not be moved. + proto_key->SetStringValue(string_key->ToStringView(&proto_key_scratch)); return true; } return false; diff --git a/common/values/struct_value_builder.cc b/common/values/struct_value_builder.cc index d1e5b4882..1cb117f46 100644 --- a/common/values/struct_value_builder.cc +++ b/common/values/struct_value_builder.cc @@ -391,8 +391,9 @@ absl::StatusOr> ProtoStringMapKeyFromValueConverter( const Value& value, google::protobuf::MapKey& key, std::string& key_string, google::protobuf::Arena* absl_nonnull arena) { if (auto string_value = value.AsString(); string_value) { - key_string = string_value->NativeString(); - key.SetStringValue(key_string); + // `google::protobuf::MapKey` does not own the string, which may be stored inline in + // `value` or in `key_string`; `value` must not be moved. + key.SetStringValue(string_value->ToStringView(&key_string)); return std::nullopt; } return TypeConversionError(value.GetTypeName(), "string", arena); diff --git a/eval/public/containers/internal_field_backed_map_impl.cc b/eval/public/containers/internal_field_backed_map_impl.cc index 2d5666c58..e4569aa82 100644 --- a/eval/public/containers/internal_field_backed_map_impl.cc +++ b/eval/public/containers/internal_field_backed_map_impl.cc @@ -174,7 +174,6 @@ absl::StatusOr FieldBackedMapImpl::LookupMapValue( return InvalidMapKeyType(key_desc_->cpp_type_name()); } - std::string map_key_string; google::protobuf::MapKey proto_key; switch (key_desc_->cpp_type()) { case google::protobuf::FieldDescriptor::CPPTYPE_BOOL: { @@ -199,8 +198,7 @@ absl::StatusOr FieldBackedMapImpl::LookupMapValue( case google::protobuf::FieldDescriptor::CPPTYPE_STRING: { CelValue::StringHolder key_value; key.GetValue(&key_value); - map_key_string.assign(key_value.value().data(), key_value.value().size()); - proto_key.SetStringValue(map_key_string); + proto_key.SetStringValue(key_value.value()); } break; case google::protobuf::FieldDescriptor::CPPTYPE_UINT32: { uint64_t key_value; diff --git a/eval/public/structs/cel_proto_wrap_util.cc b/eval/public/structs/cel_proto_wrap_util.cc index 87d05ee3f..08d716d46 100644 --- a/eval/public/structs/cel_proto_wrap_util.cc +++ b/eval/public/structs/cel_proto_wrap_util.cc @@ -111,7 +111,7 @@ class DynamicMap : public CelMap { "Invalid map key type: '", CelValue::TypeName(key.type()), "'")); } - return values_->fields().contains(std::string(str_key.value())); + return values_->fields().contains(str_key.value()); } absl::optional operator[](CelValue key) const override; @@ -692,7 +692,7 @@ absl::optional DynamicMap::operator[](CelValue key) const { CelValue::TypeName(key.type()), "'"))); } - auto it = values_->fields().find(std::string(str_key.value())); + auto it = values_->fields().find(str_key.value()); if (it == values_->fields().end()) { return std::nullopt; } diff --git a/extensions/protobuf/internal/qualify.cc b/extensions/protobuf/internal/qualify.cc index 1d53dac0d..7c1503d1c 100644 --- a/extensions/protobuf/internal/qualify.cc +++ b/extensions/protobuf/internal/qualify.cc @@ -104,7 +104,6 @@ absl::StatusOr> LookupMapValu key_desc->cpp_type_name()); } - std::string proto_key_string; google::protobuf::MapKey proto_key; switch (key_desc->cpp_type()) { case google::protobuf::FieldDescriptor::CPPTYPE_BOOL: @@ -122,8 +121,7 @@ absl::StatusOr> LookupMapValu proto_key.SetInt64Value(*key.GetInt64Key()); break; case google::protobuf::FieldDescriptor::CPPTYPE_STRING: { - proto_key_string = std::string(*key.GetStringKey()); - proto_key.SetStringValue(proto_key_string); + proto_key.SetStringValue(*key.GetStringKey()); } break; case google::protobuf::FieldDescriptor::CPPTYPE_UINT32: { uint64_t key_value = *key.GetUint64Key(); diff --git a/extensions/select_optimization.cc b/extensions/select_optimization.cc index 5244907e6..8e1a95440 100644 --- a/extensions/select_optimization.cc +++ b/extensions/select_optimization.cc @@ -270,7 +270,8 @@ absl::StatusOr MapKeyFromQualifier(const AttributeQualifier& qual, case Kind::kBool: return cel::BoolValue(*qual.GetBoolKey()); case Kind::kString: - return StringValue::From(*qual.GetStringKey(), arena); + // Safe as `qual` is valid for the lifetime of the expression/program. + return StringValue::WrapUnsafe(*qual.GetStringKey()); default: return runtime_internal::CreateNoMatchingOverloadError( cel::builtin::kIndex);