From a7087a037636f26f8312771294a21e79b67b5114 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 22 Mar 2023 08:17:08 -0700 Subject: [PATCH] Automated rollback of commit f7e6c46824510e12dbaa49bcd7dada5b3c6e0420. PiperOrigin-RevId: 518572689 --- .../protobuf/util/message_differencer.cc | 150 +++++------------- .../protobuf/util/message_differencer.h | 63 ++------ 2 files changed, 55 insertions(+), 158 deletions(-) diff --git a/src/google/protobuf/util/message_differencer.cc b/src/google/protobuf/util/message_differencer.cc index 84d79ba489..18a542a435 100644 --- a/src/google/protobuf/util/message_differencer.cc +++ b/src/google/protobuf/util/message_differencer.cc @@ -90,73 +90,6 @@ std::string PrintShortTextFormat(const google::protobuf::Message& message) { } // namespace -class MessageDifferencer::TemporaryArray { - public: - TemporaryArray(absl::Span data, - DescriptorStack* stack) - : data_(data), stack_(stack) {} - - TemporaryArray(TemporaryArray&& other) - : data_(other.data_), stack_(other.stack_) { - other.stack_ = nullptr; - } - - ~TemporaryArray() { - if (stack_) stack_->Return(data_); - } - - const absl::Span& operator*() const { return data_; } - const absl::Span* operator->() const { - return &data_; - } - - private: - absl::Span data_; - DescriptorStack* stack_; -}; - -MessageDifferencer::TemporaryArray MessageDifferencer::DescriptorStack::Create( - int size) { - while (next_ < blocks_.size()) { - auto& block = blocks_[next_]; - if (block.space() >= size) break; - if (block.size == 0) { - // Delete any empty blocks we can't use. - blocks_.erase(blocks_.begin() + next_); - } else { - // Otherwise skip the rest of it. - ++next_; - } - } - - // We need a new block. - if (next_ >= blocks_.size()) { - Block block; - int block_size = std::max(size, 512); - block.data.reset(new const FieldDescriptor*[block_size]); - block.capacity = block_size; - block.size = 0; - blocks_.push_back(std::move(block)); - ABSL_DCHECK_EQ(next_, blocks_.size() - 1); - } - - auto& block = blocks_[next_]; - auto result = absl::MakeSpan(&block.data[block.size], size); - block.size += size; - return TemporaryArray(result, this); -} - -void MessageDifferencer::DescriptorStack::Return( - absl::Span data) { - // We pop the blocks in reverse order, so it must be on the top of the stack. - ABSL_DCHECK_LT(next_, blocks_.size()); - if (blocks_[next_].size == 0) --next_; - auto& block = blocks_[next_]; - ABSL_DCHECK_GE(block.size, data.size()); - block.size -= data.size(); - ABSL_DCHECK_EQ(block.data.get() + block.size, data.data()); -} - // A reporter to report the total number of diffs. // TODO(ykzhu): we can improve this to take into account the value differencers. class NumDiffsReporter : public google::protobuf::util::MessageDifferencer::Reporter { @@ -723,17 +656,17 @@ bool MessageDifferencer::Compare(const Message& message1, } } - auto message1_fields = RetrieveFields(message1, true); - auto message2_fields = RetrieveFields(message2, false); + FieldDescriptorArray message1_fields = RetrieveFields(message1, true); + FieldDescriptorArray message2_fields = RetrieveFields(message2, false); return CompareRequestedFieldsUsingSettings(message1, message2, unpacked_any, - *message1_fields, *message2_fields, + message1_fields, message2_fields, parent_fields) && unknown_compare_result; } -MessageDifferencer::TemporaryArray MessageDifferencer::RetrieveFields( - const Message& message, bool base_message) { +FieldDescriptorArray MessageDifferencer::RetrieveFields(const Message& message, + bool base_message) { const Descriptor* descriptor = message.GetDescriptor(); tmp_message_fields_.clear(); @@ -757,26 +690,26 @@ MessageDifferencer::TemporaryArray MessageDifferencer::RetrieveFields( // each list are different. tmp_message_fields_.push_back(nullptr); - auto array = descriptor_stack_.Create(tmp_message_fields_.size()); - std::copy(tmp_message_fields_.begin(), tmp_message_fields_.end(), - array->data()); - return array; + FieldDescriptorArray message_fields(tmp_message_fields_.begin(), + tmp_message_fields_.end()); + + return message_fields; } bool MessageDifferencer::CompareRequestedFieldsUsingSettings( const Message& message1, const Message& message2, int unpacked_any, - absl::Span message1_fields, - absl::Span message2_fields, + const FieldDescriptorArray& message1_fields, + const FieldDescriptorArray& message2_fields, std::vector* parent_fields) { if (scope_ == FULL) { if (message_field_comparison_ == EQUIVALENT) { // We need to merge the field lists of both messages (i.e. // we are merely checking for a difference in field values, // rather than the addition or deletion of fields). - auto fields_union = + FieldDescriptorArray fields_union = CombineFields(message1_fields, FULL, message2_fields, FULL); return CompareWithFieldsInternal(message1, message2, unpacked_any, - *fields_union, *fields_union, + fields_union, fields_union, parent_fields); } else { // Simple equality comparison, use the unaltered field lists. @@ -797,18 +730,18 @@ bool MessageDifferencer::CompareRequestedFieldsUsingSettings( // but only the intersection for message2. This way, any fields // only present in message2 will be ignored, but any fields only // present in message1 will be marked as a difference. - auto fields_intersection = + FieldDescriptorArray fields_intersection = CombineFields(message1_fields, PARTIAL, message2_fields, PARTIAL); return CompareWithFieldsInternal(message1, message2, unpacked_any, - message1_fields, *fields_intersection, + message1_fields, fields_intersection, parent_fields); } } } -MessageDifferencer::TemporaryArray MessageDifferencer::CombineFields( - absl::Span fields1, Scope fields1_scope, - absl::Span fields2, Scope fields2_scope) { +FieldDescriptorArray MessageDifferencer::CombineFields( + const FieldDescriptorArray& fields1, Scope fields1_scope, + const FieldDescriptorArray& fields2, Scope fields2_scope) { size_t index1 = 0; size_t index2 = 0; @@ -837,16 +770,16 @@ MessageDifferencer::TemporaryArray MessageDifferencer::CombineFields( tmp_message_fields_.push_back(nullptr); - auto array = descriptor_stack_.Create(tmp_message_fields_.size()); - std::copy(tmp_message_fields_.begin(), tmp_message_fields_.end(), - array->data()); - return array; + FieldDescriptorArray combined_fields(tmp_message_fields_.begin(), + tmp_message_fields_.end()); + + return combined_fields; } bool MessageDifferencer::CompareWithFieldsInternal( const Message& message1, const Message& message2, int unpacked_any, - absl::Span message1_fields, - absl::Span message2_fields, + const FieldDescriptorArray& message1_fields, + const FieldDescriptorArray& message2_fields, std::vector* parent_fields) { bool isDifferent = false; int field_index1 = 0; @@ -871,12 +804,12 @@ bool MessageDifferencer::CompareWithFieldsInternal( // We are ignoring field1. Report the ignore and move on to // the next field in message1_fields. if (reporter_ != NULL) { - parent_fields->emplace_back(); - SpecificField& specific_field = parent_fields->back(); + SpecificField specific_field; specific_field.message1 = &message1; specific_field.message2 = &message2; specific_field.unpacked_any = unpacked_any; specific_field.field = field1; + parent_fields->push_back(specific_field); if (report_ignores_) { reporter_->ReportIgnored(message1, message2, *parent_fields); } @@ -893,8 +826,7 @@ bool MessageDifferencer::CompareWithFieldsInternal( : 1; for (int i = 0; i < count; ++i) { - parent_fields->emplace_back(); - SpecificField& specific_field = parent_fields->back(); + SpecificField specific_field; specific_field.message1 = &message1; specific_field.message2 = &message2; specific_field.unpacked_any = unpacked_any; @@ -905,6 +837,7 @@ bool MessageDifferencer::CompareWithFieldsInternal( specific_field.index = -1; } + parent_fields->push_back(specific_field); reporter_->ReportDeleted(message1, message2, *parent_fields); parent_fields->pop_back(); } @@ -922,12 +855,12 @@ bool MessageDifferencer::CompareWithFieldsInternal( // We are ignoring field2. Report the ignore and move on to // the next field in message2_fields. if (reporter_ != NULL) { - parent_fields->emplace_back(); - SpecificField& specific_field = parent_fields->back(); + SpecificField specific_field; specific_field.message1 = &message1; specific_field.message2 = &message2; specific_field.unpacked_any = unpacked_any; specific_field.field = field2; + parent_fields->push_back(specific_field); if (report_ignores_) { reporter_->ReportIgnored(message1, message2, *parent_fields); } @@ -943,8 +876,7 @@ bool MessageDifferencer::CompareWithFieldsInternal( : 1; for (int i = 0; i < count; ++i) { - parent_fields->emplace_back(); - SpecificField& specific_field = parent_fields->back(); + SpecificField specific_field; specific_field.message1 = &message1, specific_field.message2 = &message2; specific_field.unpacked_any = unpacked_any; @@ -957,6 +889,7 @@ bool MessageDifferencer::CompareWithFieldsInternal( specific_field.new_index = -1; } + parent_fields->push_back(specific_field); reporter_->ReportAdded(message1, message2, *parent_fields); parent_fields->pop_back(); } @@ -975,12 +908,12 @@ bool MessageDifferencer::CompareWithFieldsInternal( if (IsIgnored(message1, message2, field1, *parent_fields)) { // Ignore this field. Report and move on. if (reporter_ != NULL) { - parent_fields->emplace_back(); - SpecificField& specific_field = parent_fields->back(); + SpecificField specific_field; specific_field.message1 = &message1; specific_field.message2 = &message2; specific_field.unpacked_any = unpacked_any; specific_field.field = field1; + parent_fields->push_back(specific_field); if (report_ignores_) { reporter_->ReportIgnored(message1, message2, *parent_fields); } @@ -1005,12 +938,12 @@ bool MessageDifferencer::CompareWithFieldsInternal( message1, message2, unpacked_any, field1, -1, -1, parent_fields); if (reporter_ != nullptr) { - parent_fields->emplace_back(); - SpecificField& specific_field = parent_fields->back(); + SpecificField specific_field; specific_field.message1 = &message1; specific_field.message2 = &message2; specific_field.unpacked_any = unpacked_any; specific_field.field = field1; + parent_fields->push_back(specific_field); if (fieldDifferent) { reporter_->ReportModified(message1, message2, *parent_fields); isDifferent = true; @@ -1419,14 +1352,14 @@ bool MessageDifferencer::CompareFieldValueUsingParentFields( // parent_fields is used in calls to Reporter methods. if (parent_fields != NULL) { // Append currently compared field to the end of parent_fields. - parent_fields->emplace_back(); - SpecificField& specific_field = parent_fields->back(); + SpecificField specific_field; specific_field.message1 = &message1; specific_field.message2 = &message2; specific_field.unpacked_any = unpacked_any; specific_field.field = field; AddSpecificIndex(&specific_field, message1, field, index1); AddSpecificNewIndex(&specific_field, message2, field, index2); + parent_fields->push_back(specific_field); const bool compare_result = Compare(m1, m2, false, parent_fields); parent_fields->pop_back(); return compare_result; @@ -1704,8 +1637,7 @@ bool MessageDifferencer::CompareUnknownFields( } // Build the SpecificField. This is slightly complicated. - parent_field->emplace_back(); - SpecificField& specific_field = parent_field->back(); + SpecificField specific_field; specific_field.message1 = &message1; specific_field.message2 = &message2; specific_field.unknown_field_number = focus_field->number(); @@ -1733,6 +1665,7 @@ bool MessageDifferencer::CompareUnknownFields( if (IsUnknownFieldIgnored(message1, message2, specific_field, *parent_field)) { if (report_ignores_ && reporter_ != NULL) { + parent_field->push_back(specific_field); reporter_->ReportUnknownFieldIgnored(message1, message2, *parent_field); parent_field->pop_back(); } @@ -1744,13 +1677,14 @@ bool MessageDifferencer::CompareUnknownFields( if (change_type == ADDITION || change_type == DELETION || change_type == MODIFICATION) { if (reporter_ == NULL) { - parent_field->pop_back(); // We found a difference and we have no reporter. return false; } is_different = true; } + parent_field->push_back(specific_field); + switch (change_type) { case ADDITION: reporter_->ReportAdded(message1, message2, *parent_field); diff --git a/src/google/protobuf/util/message_differencer.h b/src/google/protobuf/util/message_differencer.h index fa292817b2..d0e5c72bc5 100644 --- a/src/google/protobuf/util/message_differencer.h +++ b/src/google/protobuf/util/message_differencer.h @@ -56,7 +56,6 @@ #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "absl/log/absl_check.h" -#include "absl/types/span.h" #include "google/protobuf/util/field_comparator.h" // Always include as last one, otherwise it can break compilation @@ -773,17 +772,17 @@ class PROTOBUF_EXPORT MessageDifferencer { const FieldDescriptor* field2); // Retrieve all the set fields, including extensions. - class TemporaryArray; - TemporaryArray RetrieveFields(const Message& message, bool base_message); + FieldDescriptorArray RetrieveFields(const Message& message, + bool base_message); // Combine the two lists of fields into the combined_fields output vector. // All fields present in both lists will always be included in the combined // list. Fields only present in one of the lists will only appear in the // combined list if the corresponding fields_scope option is set to FULL. - TemporaryArray CombineFields(absl::Span fields1, - Scope fields1_scope, - absl::Span fields2, - Scope fields2_scope); + FieldDescriptorArray CombineFields(const FieldDescriptorArray& fields1, + Scope fields1_scope, + const FieldDescriptorArray& fields2, + Scope fields2_scope); // Internal version of the Compare method which performs the actual // comparison. The parent_fields vector is a vector containing field @@ -803,16 +802,16 @@ class PROTOBUF_EXPORT MessageDifferencer { // CompareWithFieldsInternal. bool CompareRequestedFieldsUsingSettings( const Message& message1, const Message& message2, int unpacked_any, - absl::Span message1_fields, - absl::Span message2_fields, + const FieldDescriptorArray& message1_fields, + const FieldDescriptorArray& message2_fields, std::vector* parent_fields); // Compares the specified messages with the specified field lists. - bool CompareWithFieldsInternal( - const Message& message1, const Message& message2, int unpacked_any, - absl::Span message1_fields, - absl::Span message2_fields, - std::vector* parent_fields); + bool CompareWithFieldsInternal(const Message& message1, + const Message& message2, int unpacked_any, + const FieldDescriptorArray& message1_fields, + const FieldDescriptorArray& message2_fields, + std::vector* parent_fields); // Compares the repeated fields, and report the error. bool CompareRepeatedField(const Message& message1, const Message& message2, @@ -955,42 +954,6 @@ class PROTOBUF_EXPORT MessageDifferencer { // Reused multiple times in RetrieveFields to avoid extra allocations std::vector tmp_message_fields_; - // We use a manual stack of field descriptors to speed up the code. - // This class went through a few iterations: - // - The initial implementation had std::vector. The continuous memory - // allocation/deallocation slowed down the code. - // - It changed to use google::protobuf::Arena for all the arrays, but this caused - // unbounded memory growth on large messages. It had to be rolled back. - // - It changed to use FixedArray to avoid both of the problems from above, - // but it caused stack frame sizes to grow significantly, causing stack - // overflow on deep messages. - // - // The current implementation uses a manual stack with large blocks: - // - Amortizes memory allocation. The large blocks mean small allocations are - // bundled together. - // - Memory is returned to the descriptor stack via RAII. This prevents - // unbounded growth and allow for memory reuse reducing allocations - // further. - // - The memory is allocated in the heap, with a bit of stack for the RAII - // object. This reduces stack frame bloat. - class DescriptorStack { - public: - TemporaryArray Create(int size); - - void Return(absl::Span data); - - private: - struct Block { - std::unique_ptr data; - int space() const { return capacity - size; } - int capacity; - int size; - }; - std::vector blocks_; - size_t next_ = 0; - }; - DescriptorStack descriptor_stack_; - absl::flat_hash_set ignored_fields_; union {