From ecbb54a30535b5d08bda9802fc6c6039a65376c1 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 10 Jul 2020 06:27:16 -0400 Subject: [PATCH] protobuf: optimize unpackTo() for message upgrades. While looking at eds_speed_test flamegraphs as part of #10875, it seemed we were doing some redundant work, namely first unpacking to v2 message and then upgrading from v2 to v3. Since v2 and v3 are wire compatible, and upgrade is just a wirecast, we might as well just unpack to v2. This improves eds_speed_test significantly, the penalty for v3 vs. v2 drops from 2.6x to 2x. Part of #11362 #10875. Risk level: Low Testing: Coverage of behavior from existing tests. Signed-off-by: Harvey Tuch --- source/common/config/version_converter.cc | 24 +++++++++++------------ source/common/config/version_converter.h | 9 +++++++++ source/common/protobuf/utility.cc | 18 +++++++++-------- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc index c123a101b50e..db2bd1cfc216 100644 --- a/source/common/config/version_converter.cc +++ b/source/common/config/version_converter.cc @@ -60,10 +60,19 @@ DynamicMessagePtr createForDescriptorWithCast(const Protobuf::Message& message, return dynamic_message; } +} // namespace + +void VersionConverter::upgrade(const Protobuf::Message& prev_message, + Protobuf::Message& next_message) { + wireCast(prev_message, next_message); + // Track original type to support recoverOriginal(). + annotateWithOriginalType(*prev_message.GetDescriptor(), next_message); +} + // This needs to be recursive, since sub-messages are consumed and stored // internally, we later want to recover their original types. -void annotateWithOriginalType(const Protobuf::Descriptor& prev_descriptor, - Protobuf::Message& next_message) { +void VersionConverter::annotateWithOriginalType(const Protobuf::Descriptor& prev_descriptor, + Protobuf::Message& upgraded_message) { class TypeAnnotatingProtoVisitor : public ProtobufMessage::ProtoVisitor { public: void onMessage(Protobuf::Message& message, const void* ctxt) override { @@ -103,16 +112,7 @@ void annotateWithOriginalType(const Protobuf::Descriptor& prev_descriptor, } }; TypeAnnotatingProtoVisitor proto_visitor; - ProtobufMessage::traverseMutableMessage(proto_visitor, next_message, &prev_descriptor); -} - -} // namespace - -void VersionConverter::upgrade(const Protobuf::Message& prev_message, - Protobuf::Message& next_message) { - wireCast(prev_message, next_message); - // Track original type to support recoverOriginal(). - annotateWithOriginalType(*prev_message.GetDescriptor(), next_message); + ProtobufMessage::traverseMutableMessage(proto_visitor, upgraded_message, &prev_descriptor); } void VersionConverter::eraseOriginalTypeInformation(Protobuf::Message& message) { diff --git a/source/common/config/version_converter.h b/source/common/config/version_converter.h index 22ecc383d491..db9c76523931 100644 --- a/source/common/config/version_converter.h +++ b/source/common/config/version_converter.h @@ -91,6 +91,15 @@ class VersionConverter { static void prepareMessageForGrpcWire(Protobuf::Message& message, envoy::config::core::v3::ApiVersion api_version); + /** + * Annotate an upgraded message with original message type information. + * + * @param prev_descriptor descriptor for original type. + * @param upgraded_message upgraded message. + */ + static void annotateWithOriginalType(const Protobuf::Descriptor& prev_descriptor, + Protobuf::Message& upgraded_message); + /** * For a message that may have been upgraded, recover the original message. * This is useful for config dump, debug output etc. diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index f29c39430773..09e2fcc82f81 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -577,16 +577,18 @@ void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Messag Config::ApiTypeOracle::getEarlierVersionDescriptor(message.GetDescriptor()->full_name()); // If the earlier version matches, unpack and upgrade. if (earlier_version_desc != nullptr && any_full_name == earlier_version_desc->full_name()) { - Protobuf::DynamicMessageFactory dmf; - auto earlier_message = - ProtobufTypes::MessagePtr(dmf.GetPrototype(earlier_version_desc)->New()); - ASSERT(earlier_message != nullptr); - if (!any_message.UnpackTo(earlier_message.get())) { + // Take the Any message but adjust its type URL, since earlier/later versions are wire + // compatible. + ProtobufWkt::Any any_message_with_fixup; + any_message_with_fixup.MergeFrom(any_message); + any_message_with_fixup.set_type_url("type.googleapis.com/" + + message.GetDescriptor()->full_name()); + if (!any_message_with_fixup.UnpackTo(&message)) { throw EnvoyException(fmt::format("Unable to unpack as {}: {}", - earlier_message->GetDescriptor()->full_name(), - any_message.DebugString())); + earlier_version_desc->full_name(), + any_message_with_fixup.DebugString())); } - Config::VersionConverter::upgrade(*earlier_message, message); + Config::VersionConverter::annotateWithOriginalType(*earlier_version_desc, message); return; } }