From 9dee1ca737f168c898923884bf0503aeb1a9fc86 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Tue, 10 Jan 2023 13:42:19 -0800 Subject: [PATCH] Use LazyStringArrayList directly in gencode -- LazyStringArrayList tracks mutability internally, so we can use the current "mutable bit" as a "has bit" to potentially skip work on the field during build operations. PiperOrigin-RevId: 501083441 --- .../protobuf/compiler/java/message_builder.cc | 26 +++++++-- .../protobuf/compiler/java/string_field.cc | 58 ++++++++++--------- 2 files changed, 50 insertions(+), 34 deletions(-) diff --git a/src/google/protobuf/compiler/java/message_builder.cc b/src/google/protobuf/compiler/java/message_builder.cc index b6caea648a..78b6314bf3 100644 --- a/src/google/protobuf/compiler/java/message_builder.cc +++ b/src/google/protobuf/compiler/java/message_builder.cc @@ -73,6 +73,23 @@ std::string MapValueImmutableClassdName(const Descriptor* descriptor, GOOGLE_ABSL_CHECK_EQ(FieldDescriptor::TYPE_MESSAGE, value_field->type()); return name_resolver->GetImmutableClassName(value_field->message_type()); } + +bool BitfieldTracksMutability(const FieldDescriptor* const descriptor) { + if (!descriptor->is_repeated() || IsMapField(descriptor)) { + return false; + } + // TODO(b/255468704): update this to migrate repeated fields to use + // ProtobufList (which tracks immutability internally). That allows us to use + // the presence bit to skip work on the repeated field if it is not populated. + // Once all repeated fields are held in ProtobufLists, this method shouldn't + // be needed. + switch (descriptor->type()) { + case FieldDescriptor::TYPE_STRING: + return false; + default: + return true; + } +} } // namespace MessageBuilderGenerator::MessageBuilderGenerator(const Descriptor* descriptor, @@ -579,8 +596,7 @@ void MessageBuilderGenerator::GenerateBuildPartial(io::Printer* printer) { // Handle the repeated fields first so that the "mutable bits" are cleared. bool has_repeated_fields = false; for (int i = 0; i < descriptor_->field_count(); ++i) { - if (descriptor_->field(i)->is_repeated() && - !IsMapField(descriptor_->field(i))) { + if (BitfieldTracksMutability(descriptor_->field(i))) { has_repeated_fields = true; printer->Print("buildPartialRepeatedFields(result);\n"); break; @@ -616,8 +632,7 @@ void MessageBuilderGenerator::GenerateBuildPartial(io::Printer* printer) { "classname", name_resolver_->GetImmutableClassName(descriptor_)); printer->Indent(); for (int i = 0; i < descriptor_->field_count(); ++i) { - if (descriptor_->field(i)->is_repeated() && - !IsMapField(descriptor_->field(i))) { + if (BitfieldTracksMutability(descriptor_->field(i))) { const ImmutableFieldGenerator& field = field_generators_.get(descriptor_->field(i)); field.GenerateBuildingCode(printer); @@ -683,8 +698,7 @@ int MessageBuilderGenerator::GenerateBuildPartialPiece(io::Printer* printer, // Skip repeated fields because they are currently handled // in separate buildPartial sub-methods. - if (descriptor_->field(next)->is_repeated() && - !IsMapField(descriptor_->field(next))) { + if (BitfieldTracksMutability(descriptor_->field(next))) { continue; } // Skip fields without presence bits in the builder diff --git a/src/google/protobuf/compiler/java/string_field.cc b/src/google/protobuf/compiler/java/string_field.cc index e4d638227e..665ad6a427 100644 --- a/src/google/protobuf/compiler/java/string_field.cc +++ b/src/google/protobuf/compiler/java/string_field.cc @@ -65,7 +65,8 @@ void SetPrimitiveVariables( Context* context) { SetCommonFieldVariables(descriptor, info, variables); - (*variables)["empty_list"] = "com.google.protobuf.LazyStringArrayList.EMPTY"; + (*variables)["empty_list"] = + "com.google.protobuf.LazyStringArrayList.emptyList()"; (*variables)["default"] = ImmutableDefaultValue(descriptor, name_resolver, context->options()); @@ -121,11 +122,6 @@ void SetPrimitiveVariables( (*variables)["name"], "_)")}); } - // For repeated builders, one bit is used for whether the array is immutable. - (*variables)["get_mutable_bit_builder"] = GenerateGetBit(builderBitIndex); - (*variables)["set_mutable_bit_builder"] = GenerateSetBit(builderBitIndex); - (*variables)["clear_mutable_bit_builder"] = GenerateClearBit(builderBitIndex); - (*variables)["get_has_field_bit_builder"] = GenerateGetBit(builderBitIndex); (*variables)["get_has_field_bit_from_local"] = GenerateGetBitFromLocal(builderBitIndex); @@ -197,8 +193,7 @@ int ImmutableStringFieldGenerator::GetNumBitsForBuilder() const { return 1; } // separate fields but rather use dynamic type checking. // // For single fields, the logic for this is done inside the generated code. For -// repeated fields, the logic is done in LazyStringArrayList and -// UnmodifiableLazyStringList. +// repeated fields, the logic is done in LazyStringArrayList. void ImmutableStringFieldGenerator::GenerateInterfaceMembers( io::Printer* printer) const { if (HasHazzer(descriptor_)) { @@ -794,7 +789,8 @@ void RepeatedImmutableStringFieldGenerator::GenerateMembers( io::Printer* printer) const { printer->Print(variables_, "@SuppressWarnings(\"serial\")\n" - "private com.google.protobuf.LazyStringList $name$_;\n"); + "private com.google.protobuf.LazyStringArrayList $name$_ =\n" + " $empty_list$;\n"); PrintExtraFieldInfo(variables_, printer); WriteFieldAccessorDocComment(printer, descriptor_, LIST_GETTER); printer->Print(variables_, @@ -838,17 +834,17 @@ void RepeatedImmutableStringFieldGenerator::GenerateBuilderMembers( // memory allocations. Note, immutable is a strong guarantee here -- not // just that the list cannot be modified via the reference but that the // list can never be modified. - printer->Print( - variables_, - "private com.google.protobuf.LazyStringList $name$_ = $empty_list$;\n"); + printer->Print(variables_, + "private com.google.protobuf.LazyStringArrayList $name$_ =\n" + " $empty_list$;\n"); printer->Print( variables_, "private void ensure$capitalized_name$IsMutable() {\n" - " if (!$get_mutable_bit_builder$) {\n" + " if (!$name$_.isModifiable()) {\n" " $name$_ = new com.google.protobuf.LazyStringArrayList($name$_);\n" - " $set_mutable_bit_builder$;\n" - " }\n" + " }\n" + " $set_has_field_bit_builder$\n" "}\n"); // Note: We return an unmodifiable list because otherwise the caller @@ -859,7 +855,8 @@ void RepeatedImmutableStringFieldGenerator::GenerateBuilderMembers( printer->Print(variables_, "$deprecation$public com.google.protobuf.ProtocolStringList\n" " ${$get$capitalized_name$List$}$() {\n" - " return $name$_.getUnmodifiableView();\n" + " $name$_.makeImmutable();\n" + " return $name$_;\n" "}\n"); printer->Annotate("{", "}", descriptor_); WriteFieldAccessorDocComment(printer, descriptor_, LIST_COUNT); @@ -892,6 +889,7 @@ void RepeatedImmutableStringFieldGenerator::GenerateBuilderMembers( " $null_check$\n" " ensure$capitalized_name$IsMutable();\n" " $name$_.set(index, value);\n" + " $set_has_field_bit_builder$\n" " $on_changed$\n" " return this;\n" "}\n"); @@ -904,6 +902,7 @@ void RepeatedImmutableStringFieldGenerator::GenerateBuilderMembers( " $null_check$\n" " ensure$capitalized_name$IsMutable();\n" " $name$_.add(value);\n" + " $set_has_field_bit_builder$\n" " $on_changed$\n" " return this;\n" "}\n"); @@ -916,6 +915,7 @@ void RepeatedImmutableStringFieldGenerator::GenerateBuilderMembers( " ensure$capitalized_name$IsMutable();\n" " com.google.protobuf.AbstractMessageLite.Builder.addAll(\n" " values, $name$_);\n" + " $set_has_field_bit_builder$\n" " $on_changed$\n" " return this;\n" "}\n"); @@ -925,8 +925,9 @@ void RepeatedImmutableStringFieldGenerator::GenerateBuilderMembers( printer->Print( variables_, "$deprecation$public Builder ${$clear$capitalized_name$$}$() {\n" - " $name$_ = $empty_list$;\n" - " $clear_mutable_bit_builder$;\n" + " $name$_ =\n" + " $empty_list$;\n" + " $clear_has_field_bit_builder$;\n" " $on_changed$\n" " return this;\n" "}\n"); @@ -946,6 +947,7 @@ void RepeatedImmutableStringFieldGenerator::GenerateBuilderMembers( printer->Print(variables_, " ensure$capitalized_name$IsMutable();\n" " $name$_.add(value);\n" + " $set_has_field_bit_builder$\n" " $on_changed$\n" " return this;\n" "}\n"); @@ -1060,14 +1062,16 @@ void RepeatedImmutableStringFieldGenerator:: void RepeatedImmutableStringFieldGenerator::GenerateInitializationCode( io::Printer* printer) const { - printer->Print(variables_, "$name$_ = $empty_list$;\n"); + printer->Print(variables_, + "$name$_ =\n" + " $empty_list$;\n"); } void RepeatedImmutableStringFieldGenerator::GenerateBuilderClearCode( io::Printer* printer) const { printer->Print(variables_, - "$name$_ = $empty_list$;\n" - "$clear_mutable_bit_builder$;\n"); + "$name$_ =\n" + " $empty_list$;\n"); } void RepeatedImmutableStringFieldGenerator::GenerateMergingCode( @@ -1081,7 +1085,7 @@ void RepeatedImmutableStringFieldGenerator::GenerateMergingCode( "if (!other.$name$_.isEmpty()) {\n" " if ($name$_.isEmpty()) {\n" " $name$_ = other.$name$_;\n" - " $clear_mutable_bit_builder$;\n" + " $set_has_field_bit_builder$\n" " } else {\n" " ensure$capitalized_name$IsMutable();\n" " $name$_.addAll(other.$name$_);\n" @@ -1094,13 +1098,11 @@ void RepeatedImmutableStringFieldGenerator::GenerateBuildingCode( io::Printer* printer) const { // The code below ensures that the result has an immutable list. If our // list is immutable, we can just reuse it. If not, we make it immutable. - printer->Print(variables_, - "if ($get_mutable_bit_builder$) {\n" - " $name$_ = $name$_.getUnmodifiableView();\n" - " $clear_mutable_bit_builder$;\n" - "}\n" - "result.$name$_ = $name$_;\n"); + "if ($get_has_field_bit_from_local$) {\n" + " $name$_.makeImmutable();\n" + " result.$name$_ = $name$_;\n" + "}\n"); } void RepeatedImmutableStringFieldGenerator::GenerateBuilderParsingCode(