From 66c516cf382b94534408e8cf3e93450807b424d6 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Wed, 7 Oct 2026 10:08:46 -0700 Subject: [PATCH] Avoid copying repeated field elements onto the arena in `ParsedRepeatedFieldValue::Contains`, `ForEach`, and `NewIterator` PiperOrigin-RevId: 995185472 --- common/value.cc | 34 ++++++++++++++------ common/value.h | 2 +- common/values/parsed_repeated_field_value.cc | 14 ++++---- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/common/value.cc b/common/value.cc index bc8a3d7a0..ec606071e 100644 --- a/common/value.cc +++ b/common/value.cc @@ -935,6 +935,7 @@ void BoolRepeatedFieldAccessor( *result = BoolValue(reflection->GetRepeatedBool(*message, field, index)); } +template void StringRepeatedFieldAccessor( int index, const google::protobuf::Message* absl_nonnull message, const google::protobuf::FieldDescriptor* absl_nonnull field, @@ -963,6 +964,8 @@ void StringRepeatedFieldAccessor( if (string.data() == scratch.data() && string.size() == scratch.size()) { *result = StringValue::From(std::move(scratch), arena); + } else if constexpr (Unsafe) { + *result = StringValue::WrapUnsafe(string); } else { if (message->GetArena() == nullptr) { *result = StringValue::From(string, arena); @@ -975,9 +978,10 @@ void StringRepeatedFieldAccessor( *result = StringValue::From(std::move(cord), arena); }), well_known_types::AsVariant(well_known_types::GetRepeatedStringField( - *message, field, index, scratch))); + reflection, *message, field, index, scratch))); } +template void MessageRepeatedFieldAccessor( int index, const google::protobuf::Message* absl_nonnull message, const google::protobuf::FieldDescriptor* absl_nonnull field, @@ -999,11 +1003,18 @@ void MessageRepeatedFieldAccessor( ABSL_DCHECK_GE(index, 0); ABSL_DCHECK_LT(index, reflection->FieldSize(*message, field)); - *result = Value::WrapMessage( - &reflection->GetRepeatedMessage(*message, field, index), descriptor_pool, - message_factory, arena); + if constexpr (Unsafe) { + *result = Value::WrapMessageUnsafe( + &reflection->GetRepeatedMessage(*message, field, index), + descriptor_pool, message_factory, arena); + } else { + *result = Value::WrapMessage( + &reflection->GetRepeatedMessage(*message, field, index), + descriptor_pool, message_factory, arena); + } } +template void BytesRepeatedFieldAccessor( int index, const google::protobuf::Message* absl_nonnull message, const google::protobuf::FieldDescriptor* absl_nonnull field, @@ -1032,6 +1043,8 @@ void BytesRepeatedFieldAccessor( if (string.data() == scratch.data() && string.size() == scratch.size()) { *result = BytesValue::From(std::move(scratch), arena); + } else if constexpr (Unsafe) { + *result = BytesValue::WrapUnsafe(string); } else { if (message->GetArena() == nullptr) { *result = BytesValue::From(string, arena); @@ -1044,7 +1057,7 @@ void BytesRepeatedFieldAccessor( *result = BytesValue::From(std::move(cord), arena); }), well_known_types::AsVariant(well_known_types::GetRepeatedBytesField( - *message, field, index, scratch))); + reflection, *message, field, index, scratch))); } void EnumRepeatedFieldAccessor( @@ -1101,7 +1114,7 @@ void NullRepeatedFieldAccessor( } // namespace absl::StatusOr RepeatedFieldAccessorFor( - const google::protobuf::FieldDescriptor* absl_nonnull field) { + const google::protobuf::FieldDescriptor* absl_nonnull field, bool unsafe) { switch (field->type()) { case google::protobuf::FieldDescriptor::TYPE_DOUBLE: return &DoubleRepeatedFieldAccessor; @@ -1126,13 +1139,16 @@ absl::StatusOr RepeatedFieldAccessorFor( case google::protobuf::FieldDescriptor::TYPE_BOOL: return &BoolRepeatedFieldAccessor; case google::protobuf::FieldDescriptor::TYPE_STRING: - return &StringRepeatedFieldAccessor; + return unsafe ? &StringRepeatedFieldAccessor + : &StringRepeatedFieldAccessor; case google::protobuf::FieldDescriptor::TYPE_GROUP: ABSL_FALLTHROUGH_INTENDED; case google::protobuf::FieldDescriptor::TYPE_MESSAGE: - return &MessageRepeatedFieldAccessor; + return unsafe ? &MessageRepeatedFieldAccessor + : &MessageRepeatedFieldAccessor; case google::protobuf::FieldDescriptor::TYPE_BYTES: - return &BytesRepeatedFieldAccessor; + return unsafe ? &BytesRepeatedFieldAccessor + : &BytesRepeatedFieldAccessor; case google::protobuf::FieldDescriptor::TYPE_FIXED32: ABSL_FALLTHROUGH_INTENDED; case google::protobuf::FieldDescriptor::TYPE_UINT32: diff --git a/common/value.h b/common/value.h index 9278cf4de..7d25efcc5 100644 --- a/common/value.h +++ b/common/value.h @@ -2940,7 +2940,7 @@ using RepeatedFieldAccessor = Value* absl_nonnull); absl::StatusOr RepeatedFieldAccessorFor( - const google::protobuf::FieldDescriptor* absl_nonnull field); + const google::protobuf::FieldDescriptor* absl_nonnull field, bool unsafe = false); } // namespace common_internal diff --git a/common/values/parsed_repeated_field_value.cc b/common/values/parsed_repeated_field_value.cc index b716962cc..c50dd53c1 100644 --- a/common/values/parsed_repeated_field_value.cc +++ b/common/values/parsed_repeated_field_value.cc @@ -273,8 +273,9 @@ absl::Status ParsedRepeatedFieldValue::ForEach( const auto* reflection = message_->GetReflection(); const int size = reflection->FieldSize(*message_, field_); if (size > 0) { - CEL_ASSIGN_OR_RETURN(auto accessor, - common_internal::RepeatedFieldAccessorFor(field_)); + CEL_ASSIGN_OR_RETURN( + auto accessor, + common_internal::RepeatedFieldAccessorFor(field_, arena_ == nullptr)); Value scratch; for (int i = 0; i < size; ++i) { (*accessor)(i, message_, field_, reflection, descriptor_pool, @@ -377,8 +378,8 @@ ParsedRepeatedFieldValue::NewIterator() const { if (ABSL_PREDICT_FALSE(field_ == nullptr)) { return NewEmptyValueIterator(); } - CEL_ASSIGN_OR_RETURN(auto accessor, - common_internal::RepeatedFieldAccessorFor(field_)); + CEL_ASSIGN_OR_RETURN(auto accessor, common_internal::RepeatedFieldAccessorFor( + field_, arena_ == nullptr)); return std::make_unique(message_, field_, accessor); } @@ -396,8 +397,9 @@ absl::Status ParsedRepeatedFieldValue::Contains( const auto* reflection = message_->GetReflection(); const int size = reflection->FieldSize(*message_, field_); if (size > 0) { - CEL_ASSIGN_OR_RETURN(auto accessor, - common_internal::RepeatedFieldAccessorFor(field_)); + CEL_ASSIGN_OR_RETURN( + auto accessor, + common_internal::RepeatedFieldAccessorFor(field_, /*unsafe=*/true)); Value scratch; for (int i = 0; i < size; ++i) { (*accessor)(i, message_, field_, reflection, descriptor_pool,