From b5f9a35b165a44ea9ca1bcfe91d887b378b6441d Mon Sep 17 00:00:00 2001 From: Hao Nguyen <45579440+haon4@users.noreply.github.com> Date: Wed, 2 Jan 2019 15:10:38 -0800 Subject: [PATCH] Down-integrate internal changes to github. (#5527) --- .../protobuf/compiler/js/js_generator.cc | 39 ++++++----- .../protobuf/generated_message_reflection.cc | 4 +- src/google/protobuf/map_field.cc | 58 ++++++++++++++--- src/google/protobuf/map_field.h | 8 ++- src/google/protobuf/map_field_inl.h | 10 ++- src/google/protobuf/map_field_test.cc | 13 ++-- src/google/protobuf/map_test_util.cc | 4 ++ .../util/internal/protostream_objectsource.cc | 19 +++--- .../util/internal/protostream_objectsource.h | 9 ++- .../protobuf/util/type_resolver_util.cc | 41 +++++++++--- .../protobuf/util/type_resolver_util_test.cc | 65 +++++++++++++++++-- 11 files changed, 203 insertions(+), 67 deletions(-) diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc index db19ff58dd..06383936de 100644 --- a/src/google/protobuf/compiler/js/js_generator.cc +++ b/src/google/protobuf/compiler/js/js_generator.cc @@ -2848,44 +2848,50 @@ void Generator::GenerateClassField(const GeneratorOptions& options, // fields with presence. if (field->is_map()) { printer->Print( + "/** Clears values from the map. The map will be non-null. */\n" "$class$.prototype.$clearername$ = function() {\n" " this.$gettername$().clear();$returnvalue$\n" "};\n" "\n" "\n", "class", GetMessagePath(options, field->containing_type()), - "clearername", "clear" + JSGetterName(options, field), - "gettername", "get" + JSGetterName(options, field), - "returnvalue", JSReturnClause(field)); + "clearername", "clear" + JSGetterName(options, field), "gettername", + "get" + JSGetterName(options, field), "returnvalue", + JSReturnClause(field)); printer->Annotate("clearername", field); } else if (field->is_repeated() || (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE && !field->is_required())) { // Fields where we can delegate to the regular setter. printer->Print( + "/** $jsdoc$ */\n" "$class$.prototype.$clearername$ = function() {\n" " this.$settername$($clearedvalue$);$returnvalue$\n" "};\n" "\n" "\n", + "jsdoc", + field->is_repeated() ? "Clears the list making it empty but non-null." + : "Clears the message field making it undefined.", "class", GetMessagePath(options, field->containing_type()), - "clearername", "clear" + JSGetterName(options, field), - "settername", "set" + JSGetterName(options, field), - "clearedvalue", (field->is_repeated() ? "[]" : "undefined"), - "returnvalue", JSReturnClause(field)); + "clearername", "clear" + JSGetterName(options, field), "settername", + "set" + JSGetterName(options, field), "clearedvalue", + (field->is_repeated() ? "[]" : "undefined"), "returnvalue", + JSReturnClause(field)); printer->Annotate("clearername", field); } else if (HasFieldPresence(options, field)) { // Fields where we can't delegate to the regular setter because it doesn't // accept "undefined" as an argument. printer->Print( + "/** Clears the field making it undefined. */\n" "$class$.prototype.$clearername$ = function() {\n" " jspb.Message.set$maybeoneof$Field(this, " "$index$$maybeoneofgroup$, ", "class", GetMessagePath(options, field->containing_type()), - "clearername", "clear" + JSGetterName(options, field), - "maybeoneof", (field->containing_oneof() ? "Oneof" : ""), - "maybeoneofgroup", (field->containing_oneof() ? - (", " + JSOneofArray(options, field)) : ""), + "clearername", "clear" + JSGetterName(options, field), "maybeoneof", + (field->containing_oneof() ? "Oneof" : ""), "maybeoneofgroup", + (field->containing_oneof() ? (", " + JSOneofArray(options, field)) + : ""), "index", JSFieldIndex(field)); printer->Annotate("clearername", field); printer->Print( @@ -2963,14 +2969,15 @@ void Generator::GenerateRepeatedMessageHelperMethods( " * @param {number=} opt_index\n" " * @return {!$optionaltype$}\n" " */\n" - "$class$.prototype.add$name$ = function(opt_value, opt_index) {\n" + "$class$.prototype.$addername$ = function(opt_value, opt_index) {\n" " return jspb.Message.addTo$repeatedtag$WrapperField(", - "optionaltype", JSTypeName(options, field, BYTES_DEFAULT), - "class", GetMessagePath(options, field->containing_type()), - "name", JSGetterName(options, field, BYTES_DEFAULT, - /* drop_list = */ true), + "optionaltype", JSTypeName(options, field, BYTES_DEFAULT), "class", + GetMessagePath(options, field->containing_type()), "addername", + "add" + JSGetterName(options, field, BYTES_DEFAULT, + /* drop_list = */ true), "repeatedtag", (field->is_repeated() ? "Repeated" : "")); + printer->Annotate("addername", field); printer->Print( "this, $index$$oneofgroup$, opt_value, $ctor$, opt_index);\n" "};\n" diff --git a/src/google/protobuf/generated_message_reflection.cc b/src/google/protobuf/generated_message_reflection.cc index 96852ea7c8..e77026c782 100644 --- a/src/google/protobuf/generated_message_reflection.cc +++ b/src/google/protobuf/generated_message_reflection.cc @@ -867,9 +867,7 @@ void GeneratedMessageReflection::ClearField( case FieldDescriptor::CPPTYPE_MESSAGE: { if (IsMapFieldInApi(field)) { - MutableRaw(message, field) - ->MutableRepeatedField() - ->Clear >(); + MutableRaw(message, field)->Clear(); } else { // We don't know which subclass of RepeatedPtrFieldBase the type is, // so we use RepeatedPtrFieldBase directly. diff --git a/src/google/protobuf/map_field.cc b/src/google/protobuf/map_field.cc index 7c10d67f4c..9b27a32e7e 100644 --- a/src/google/protobuf/map_field.cc +++ b/src/google/protobuf/map_field.cc @@ -91,21 +91,47 @@ void MapFieldBase::SetRepeatedDirty() { state_.store(STATE_MODIFIED_REPEATED, std::memory_order_relaxed); } +void MapFieldBase::SetClean() { + // These are called by (non-const) mutator functions. So by our API it's the + // callers responsibility to have these calls properly ordered. + state_.store(CLEAN, std::memory_order_relaxed); +} + void* MapFieldBase::MutableRepeatedPtrField() const { return repeated_field_; } void MapFieldBase::SyncRepeatedFieldWithMap() const { // acquire here matches with release below to ensure that we can only see a // value of CLEAN after all previous changes have been synced. - if (state_.load(std::memory_order_acquire) == STATE_MODIFIED_MAP) { - mutex_.Lock(); - // Double check state, because another thread may have seen the same state - // and done the synchronization before the current thread. - if (state_.load(std::memory_order_relaxed) == STATE_MODIFIED_MAP) { - SyncRepeatedFieldWithMapNoLock(); - state_.store(CLEAN, std::memory_order_release); + switch (state_.load(std::memory_order_acquire)) { + case STATE_MODIFIED_MAP: + mutex_.Lock(); + // Double check state, because another thread may have seen the same + // state and done the synchronization before the current thread. + if (state_.load(std::memory_order_relaxed) == STATE_MODIFIED_MAP) { + SyncRepeatedFieldWithMapNoLock(); + state_.store(CLEAN, std::memory_order_release); + } + mutex_.Unlock(); + break; + case CLEAN: + mutex_.Lock(); + // Double check state + if (state_.load(std::memory_order_relaxed) == CLEAN) { + if (repeated_field_ == nullptr) { + if (arena_ == nullptr) { + repeated_field_ = new RepeatedPtrField(); + } else { + repeated_field_ = + Arena::CreateMessage >(arena_); + } + } + state_.store(CLEAN, std::memory_order_release); + } + mutex_.Unlock(); + break; + default: + break; } - mutex_.Unlock(); - } } void MapFieldBase::SyncRepeatedFieldWithMapNoLock() const { @@ -155,6 +181,20 @@ int DynamicMapField::size() const { return GetMap().size(); } +void DynamicMapField::Clear() { + Map* map = &const_cast(this)->map_; + for (Map::iterator iter = map->begin(); + iter != map->end(); ++iter) { + iter->second.DeleteData(); + } + map->clear(); + + if (MapFieldBase::repeated_field_ != nullptr) { + MapFieldBase::repeated_field_->Clear(); + } + MapFieldBase::SetClean(); +} + bool DynamicMapField::ContainsMapKey( const MapKey& map_key) const { const Map& map = GetMap(); diff --git a/src/google/protobuf/map_field.h b/src/google/protobuf/map_field.h index 5e598b8e0d..0bb559804f 100644 --- a/src/google/protobuf/map_field.h +++ b/src/google/protobuf/map_field.h @@ -107,6 +107,7 @@ class PROTOBUF_EXPORT MapFieldBase { virtual void Swap(MapFieldBase* other) = 0; // Sync Map with repeated field and returns the size of map. virtual int size() const = 0; + virtual void Clear() = 0; // Returns the number of bytes used by the repeated field, excluding // sizeof(*this) @@ -136,6 +137,9 @@ class PROTOBUF_EXPORT MapFieldBase { // Tells MapFieldBase that there is new change to RepeatedPTrField. void SetRepeatedDirty(); + // Tells MapFieldBase that map and repeated are the same. + void SetClean(); + // Provides derived class the access to repeated field. void* MutableRepeatedPtrField() const; @@ -268,9 +272,8 @@ class MapField : public TypeDefinedMapFieldBase { return result; } - // Convenient methods for generated message implementation. int size() const override; - void Clear(); + void Clear() override; void MergeFrom(const MapFieldBase& other) override; void Swap(MapFieldBase* other) override; @@ -334,6 +337,7 @@ class PROTOBUF_EXPORT DynamicMapField Map* MutableMap() override; int size() const override; + void Clear() override; private: Map map_; diff --git a/src/google/protobuf/map_field_inl.h b/src/google/protobuf/map_field_inl.h index 73c16ed4ee..40ceb50499 100644 --- a/src/google/protobuf/map_field_inl.h +++ b/src/google/protobuf/map_field_inl.h @@ -178,9 +178,15 @@ template void MapField::Clear() { - MapFieldBase::SyncMapWithRepeatedField(); + if (this->MapFieldBase::repeated_field_ != nullptr) { + RepeatedPtrField* repeated_field = + reinterpret_cast*>( + this->MapFieldBase::repeated_field_); + repeated_field->Clear(); + } + impl_.MutableMap()->clear(); - MapFieldBase::SetMapDirty(); + MapFieldBase::SetClean(); } template size()); + if (repeated_field == nullptr) { + EXPECT_EQ(repeated_size, 0); + } else { + EXPECT_EQ(repeated_size, repeated_field->size()); + } } } @@ -442,11 +447,7 @@ TEST_P(MapFieldStateTest, SwapRepeatedDirty) { TEST_P(MapFieldStateTest, Clear) { map_field_->Clear(); - if (state_ != MAP_DIRTY) { - Expect(map_field_.get(), MAP_DIRTY, 0, 1, false); - } else { - Expect(map_field_.get(), MAP_DIRTY, 0, 0, true); - } + Expect(map_field_.get(), CLEAN, 0, 0, false); } TEST_P(MapFieldStateTest, SpaceUsedExcludingSelf) { diff --git a/src/google/protobuf/map_test_util.cc b/src/google/protobuf/map_test_util.cc index 31ac173633..25f027f305 100644 --- a/src/google/protobuf/map_test_util.cc +++ b/src/google/protobuf/map_test_util.cc @@ -1582,6 +1582,10 @@ void MapReflectionTester::ExpectClearViaReflection( EXPECT_EQ(0, reflection->FieldSize(message, F("map_int32_bytes"))); EXPECT_EQ(0, reflection->FieldSize(message, F("map_int32_enum"))); EXPECT_EQ(0, reflection->FieldSize(message, F("map_int32_foreign_message"))); + EXPECT_TRUE(reflection->GetMapData( + message, F("map_int32_foreign_message"))->IsMapValid()); + EXPECT_TRUE(reflection->GetMapData( + message, F("map_int32_foreign_message"))->IsRepeatedFieldValid()); } void MapReflectionTester::ExpectClearViaReflectionIterator( diff --git a/src/google/protobuf/util/internal/protostream_objectsource.cc b/src/google/protobuf/util/internal/protostream_objectsource.cc index a11d983751..2c1a739300 100644 --- a/src/google/protobuf/util/internal/protostream_objectsource.cc +++ b/src/google/protobuf/util/internal/protostream_objectsource.cc @@ -188,10 +188,10 @@ Status ProtoStreamObjectSource::WriteMessage(const google::protobuf::Type& type, bool include_start_and_end, ObjectWriter* ow) const { - const TypeRenderer* type_renderer = FindTypeRenderer(type.name()); - if (type_renderer != nullptr) { - return (*type_renderer)(this, type, name, ow); - } + const TypeRenderer* type_renderer = FindTypeRenderer(type.name()); + if (type_renderer != nullptr) { + return (*type_renderer)(this, type, name, ow); + } const google::protobuf::Field* field = nullptr; string field_name; @@ -229,9 +229,7 @@ Status ProtoStreamObjectSource::WriteMessage(const google::protobuf::Type& type, if (field->cardinality() == google::protobuf::Field_Cardinality_CARDINALITY_REPEATED) { - bool check_maps = true; - - if (check_maps && IsMap(*field)) { + if (IsMap(*field)) { ow->StartObject(field_name); ASSIGN_OR_RETURN(tag, RenderMap(field, field_name, tag, ow)); ow->EndObject(); @@ -332,6 +330,7 @@ Status ProtoStreamObjectSource::RenderPacked( return util::Status(); } + Status ProtoStreamObjectSource::RenderTimestamp( const ProtoStreamObjectSource* os, const google::protobuf::Type& type, StringPiece field_name, ObjectWriter* ow) { @@ -708,6 +707,7 @@ std::unordered_map* ProtoStreamObjectSource::renderers_ = NULL; PROTOBUF_NAMESPACE_ID::internal::once_flag source_renderers_init_; + void ProtoStreamObjectSource::InitRendererMap() { renderers_ = new std::unordered_map(); @@ -745,6 +745,7 @@ void ProtoStreamObjectSource::InitRendererMap() { ::google::protobuf::internal::OnShutdown(&DeleteRendererMap); } + void ProtoStreamObjectSource::DeleteRendererMap() { delete ProtoStreamObjectSource::renderers_; renderers_ = NULL; @@ -781,10 +782,8 @@ Status ProtoStreamObjectSource::RenderField( // Short-circuit any special type rendering to save call-stack space. const TypeRenderer* type_renderer = FindTypeRenderer(type->name()); - bool use_type_renderer = type_renderer != nullptr; - RETURN_IF_ERROR(IncrementRecursionDepth(type->name(), field_name)); - if (use_type_renderer) { + if (type_renderer != nullptr) { RETURN_IF_ERROR((*type_renderer)(this, *type, field_name, ow)); } else { RETURN_IF_ERROR(WriteMessage(*type, field_name, 0, true, ow)); diff --git a/src/google/protobuf/util/internal/protostream_objectsource.h b/src/google/protobuf/util/internal/protostream_objectsource.h index 124a36a1a5..ac94b038ff 100644 --- a/src/google/protobuf/util/internal/protostream_objectsource.h +++ b/src/google/protobuf/util/internal/protostream_objectsource.h @@ -46,6 +46,7 @@ #include #include + #include namespace google { @@ -181,9 +182,10 @@ class PROTOBUF_EXPORT ProtoStreamObjectSource : public ObjectSource { // Renders a NWP map. // Returns the next tag after reading all map entries. The caller should use // this tag before reading more tags from the stream. - util::StatusOr RenderMap(const google::protobuf::Field* field, - StringPiece name, uint32 list_tag, - ObjectWriter* ow) const; + util::StatusOr + RenderMap(const google::protobuf::Field* field, + StringPiece name, uint32 list_tag, + ObjectWriter* ow) const; // Renders a packed repeating field. A packed field is stored as: // {tag length item1 item2 item3} instead of the less efficient @@ -191,6 +193,7 @@ class PROTOBUF_EXPORT ProtoStreamObjectSource : public ObjectSource { util::Status RenderPacked(const google::protobuf::Field* field, ObjectWriter* ow) const; + // Renders a google.protobuf.Timestamp value to ObjectWriter static util::Status RenderTimestamp(const ProtoStreamObjectSource* os, const google::protobuf::Type& type, diff --git a/src/google/protobuf/util/type_resolver_util.cc b/src/google/protobuf/util/type_resolver_util.cc index f40ce941e9..4d7a3c0753 100644 --- a/src/google/protobuf/util/type_resolver_util.cc +++ b/src/google/protobuf/util/type_resolver_util.cc @@ -118,6 +118,27 @@ class DescriptorPoolTypeResolver : public TypeResolver { void ConvertMessageOptions(const MessageOptions& options, RepeatedPtrField